GitSCM: detect patch conflicts on apply_patch_git (bug 2030211)

src/lando/main/scm/git.py

@@ -180,6 +180,7 @@ def cherry_pick_commit(self, commit_id: str):
         self._git_run("cherry-pick", commit_id, cwd=self.path)
 
     @override
+    @detect_patch_conflict
     def apply_patch_git(self, patch_bytes: bytes):
         """Apply the Git patch, provided as encoded bytes."""
         try:

src/lando/main/tests/test_git.py

@@ -6,13 +6,18 @@
 from collections.abc import Callable
 from pathlib import Path
 from textwrap import dedent
+from typing import Any
 from unittest import mock
 from unittest.mock import MagicMock, PropertyMock
 
 import pytest
 
 from lando.main.scm.consts import MergeStrategy
-from lando.main.scm.exceptions import SCMException, TagAlreadyPresentException
+from lando.main.scm.exceptions import (
+    PatchConflict,
+    SCMException,
+    TagAlreadyPresentException,
+)
 from lando.main.scm.git import GitSCM
 from lando.main.scm.helpers import GitPatchHelper
 
@@ -58,14 +63,14 @@ def test_GitSCM_repo_is_supported(repo_path: str, expected: bool, git_repo: Path
 
 def test_GitSCM_clone(
     git_repo: Path,
-    monkeypatch: pytest.MonkeyPatch,
+    active_mock: Callable,
     request: pytest.FixtureRequest,
     tmp_path: Path,
 ):
     clone_path = tmp_path / request.node.name
     scm = GitSCM(str(clone_path))
 
-    mock_git_run = _monkeypatch_scm(monkeypatch, scm, "_git_run")
+    mock_git_run = active_mock(scm, "_git_run")
 
     scm.clone(str(git_repo))
 
@@ -83,7 +88,7 @@ def test_GitSCM_clone(
 def test_GitSCM_clean_repo(
     git_repo: Path,
     git_setup_user: Callable,
-    monkeypatch: pytest.MonkeyPatch,
+    active_mock: Callable,
     request: pytest.FixtureRequest,
     tmp_path: Path,
     create_git_commit: Callable,
@@ -118,7 +123,7 @@ def test_GitSCM_clean_repo(
     new_untracked_file = clone_path / "new_untracked_file"
     new_untracked_file.write_text("test", encoding="utf-8")
 
-    mock_git_run = _monkeypatch_scm(monkeypatch, scm, "_git_run")
+    mock_git_run = active_mock(scm, "_git_run")
 
     scm.clean_repo(strip_non_public_commits=strip_non_public_commits)
 
@@ -638,7 +643,7 @@ def test_GitSCM_changeset_descriptions_on_workbranch(
 def test_GitSCM_push(
     git_repo: Path,
     git_setup_user: Callable,
-    monkeypatch: pytest.MonkeyPatch,
+    active_mock: Callable,
     push_target: str | None,
     request: pytest.FixtureRequest,
     tmp_path: Path,
@@ -658,7 +663,7 @@ def test_GitSCM_push(
     new_untracked_file = clone_path / "new_untracked_file"
     new_untracked_file.write_text("test", encoding="utf-8")
 
-    mock_git_run = _monkeypatch_scm(monkeypatch, scm, "_git_run")
+    mock_git_run = active_mock(scm, "_git_run")
 
     scm.push(str(git_repo), push_target)
 
@@ -726,21 +731,6 @@ def test_GitSCM_git_run_redact_url_userinfo(
         assert "[REDACTED]" not in str(exc.value)
 
 
-def _monkeypatch_scm(monkeypatch, scm: GitSCM, method: str) -> MagicMock:
-    """
-    Mock a method on `scm` to test the call, but let it continue with its original side
-    effect, so we can test that it's correct, too.
-
-    Returns:
-    MagicMock: The mock object.
-    """
-    original = scm.__getattribute__(method)
-    mock = MagicMock()
-    mock.side_effect = original
-    monkeypatch.setattr(scm, method, mock)
-    return mock
-
-
 @pytest.mark.parametrize("strategy", [None, MergeStrategy.OURS, MergeStrategy.THEIRS])
 def test_GitSCM_merge_onto(
     git_repo: Path,
@@ -1207,3 +1197,55 @@ def test_GitSCM_commit_exists(
     assert not scm.commit_exists(
         "this-is-not-a-valid-commit"
     ), "`commit_exists` should return `False` for invalid commit reference."
+
+
+@pytest.mark.parametrize(
+    "method_name, method_args",
+    (
+        ("add_diff_from_patches", {"patches": ""}),
+        (
+            "apply_patch",
+            {
+                "diff": "",
+                "commit_description": "",
+                "commit_author": "",
+                "commit_date": "",
+            },
+        ),
+        ("apply_patch_git", {"patch_bytes": ""}),
+    ),
+)
+def test_GitSCM__detect_patch_conflict(
+    git_repo: Path,
+    git_setup_user: Callable,
+    request: pytest.FixtureRequest,
+    tmp_path: Path,
+    method_name: str,
+    method_args: dict[str, Any],
+):
+    """Tests that select methods correctly transform generic SCMException into PatchConflict."""
+    clone_path = tmp_path / request.node.name
+    clone_path.mkdir()
+
+    scm = GitSCM(str(clone_path))
+    scm.clone(str(git_repo))
+    git_setup_user(str(clone_path))
+
+    scm._git_run = mock.MagicMock()
+    scm._git_run.side_effect = SCMException(
+        msg="can't apply",
+        out="out",
+        err=dedent("""\
+        error: patch failed: security/manager/tools/PreloadedHPKPins.json:25
+        error: security/manager/tools/PreloadedHPKPins.json: patch does not apply
+        error: patch failed: taskcluster/docker/periodic-updates/scripts/genHPKPStaticPins.js:121
+        error: taskcluster/docker/periodic-updates/scripts/genHPKPStaticPins.js: patch does not apply
+        error: patch failed: taskcluster/docker/periodic-updates/scripts/getHSTSPreloadList.js:17
+        error: taskcluster/docker/periodic-updates/scripts/getHSTSPreloadList.js: patch does not apply
+        """).strip(),
+    )
+
+    with pytest.raises(PatchConflict) as exc_info:
+        getattr(scm, method_name)(**method_args)
+
+    assert exc_info.match("patch failed: security/manager/tools/PreloadedHPKPins.json")