Merge pull request #962 from GitGuardian/agateau/scan-all-merge-files

Change behavior of `secret scan pre-commit` on merge commits

changelog.d/20240819_162411_fnareoh_scrt_4759_optimize_files_to_scan_on_merge_commits.md

@@ -1,5 +1,3 @@
 ### Added
 
-- The command `ggshield secret scan pre-commit` has a new flag `--skip-unchanged-merge-files`. It is off by default, if activated,
-  in the case of merge commit, it skips the scan of files that were not modified by merge. This is done for efficiency
-  but is less secure.
+- When scanning a merge commit, `ggshield secret scan pre-commit` now skips files that merged without conflicts. This makes merging the default branch into a topic branch much faster. You can use the `--scan-all-merge-files` option to go back to the previous behavior.

ggshield/cmd/secret/scan/precommit.py

@@ -40,18 +40,17 @@ def check_is_merge_with_conflict(cwd: Path) -> bool:
 
 @click.command()
 @click.option(
-    "--skip-unchanged-merge-files",
+    "--scan-all-merge-files",
     is_flag=True,
-    help="When scanning a merge commit, skip files that were not modified by the merge"
-    " (assumes the merged commits are secret free).",
+    help="When scanning a merge commit, scan all files, including those that merged without conflicts.",
 )
 @click.argument("precommit_args", nargs=-1, type=click.UNPROCESSED)
 @add_secret_scan_common_options()
 @click.pass_context
 @exception_wrapper
 def precommit_cmd(
     ctx: click.Context,
-    skip_unchanged_merge_files: bool,
+    scan_all_merge_files: bool,
     precommit_args: List[str],
     **kwargs: Any,
 ) -> int:  # pragma: no cover
@@ -82,9 +81,9 @@ def precommit_cmd(
     )
 
     # Get the commit object
-    if skip_unchanged_merge_files and check_is_merge_with_conflict(Path.cwd()):
+    if not scan_all_merge_files and check_is_merge_with_conflict(Path.cwd()):
         commit = Commit.from_merge(ctx_obj.exclusion_regexes)
-    elif skip_unchanged_merge_files and check_is_merge_without_conflict():
+    elif not scan_all_merge_files and check_is_merge_without_conflict():
         merge_branch = get_merge_branch_from_reflog()
         commit = Commit.from_merge(ctx_obj.exclusion_regexes, merge_branch)
     else:

tests/functional/secret/test_merge_commit.py

@@ -25,7 +25,7 @@
     ],
 )
 @pytest.mark.parametrize(
-    "merge_skip_unchanged",
+    "scan_all_merge_files",
     [
         True,
         False,
@@ -36,20 +36,20 @@ def test_merge_commit_no_conflict(
     tmp_path: Path,
     with_conflict: bool,
     secret_location: SecretLocation,
-    merge_skip_unchanged: bool,
+    scan_all_merge_files: bool,
 ) -> None:
 
     if (
         secret_location == SecretLocation.MASTER_BRANCH
         and with_conflict
-        and not merge_skip_unchanged
+        and scan_all_merge_files
     ):
         with pytest.raises(CalledProcessError):
             generate_repo_with_merge_commit(
                 tmp_path,
                 with_conflict=with_conflict,
                 secret_location=secret_location,
-                merge_skip_unchanged=merge_skip_unchanged,
+                scan_all_merge_files=scan_all_merge_files,
             )
 
         # AND the error message contains the Gitlab Token
@@ -60,7 +60,7 @@ def test_merge_commit_no_conflict(
             tmp_path,
             with_conflict=with_conflict,
             secret_location=secret_location,
-            merge_skip_unchanged=merge_skip_unchanged,
+            scan_all_merge_files=scan_all_merge_files,
         )
 
 

tests/functional/utils_create_merge_repo.py

@@ -121,7 +121,7 @@ def generate_repo_with_merge_commit(
     nb_files_per_commit: int = DEFAULT_FILE_COUNT,
     with_conflict: bool = False,
     secret_location: SecretLocation = SecretLocation.NO_SECRET,
-    merge_skip_unchanged: bool = True,
+    scan_all_merge_files: bool = False,
 ) -> None:
     Path(root_dir).mkdir(parents=True, exist_ok=True)
     repo = Repository.create(root_dir, initial_branch="master")
@@ -177,14 +177,14 @@ def generate_repo_with_merge_commit(
         "pre-commit",
         cwd=repo.path,
     )
-    if merge_skip_unchanged:
+    if scan_all_merge_files:
         # rewrite the git hook file to add the option --skip-unchanged-merge-files
         hook_path = Path(
             f"{root_dir}/.git/hooks/pre-commit",
         )
         with open(hook_path, "r") as f:
             hook = f.read()
-        hook = hook.replace(r"pre-commit", r"pre-commit --skip-unchanged-merge-files")
+        hook = hook.replace(r"pre-commit", r"pre-commit --scan-all-merge-files")
         with open(hook_path, "w") as f:
             f.write(hook)