landing_worker: fix handling of merge conflicts (bug 1938162) (bug 19…

…35549) (#182)

test_landings: add test for merge conflicts (bug 1935549)
landing_worker: don't double-decode reject paths (bug 1938162)
scm: fix reject_path interface (bug 1938162)
landing_worker: remove content from base error_breakdown for conflicts 
abstract_scm: rename reject_paths to (get_)rejects_paths
landing_worker: better naming in process_merge_conflict

src/lando/api/legacy/spec/swagger.yml

@@ -416,7 +416,7 @@ definitions:
         description: List of paths that failed to merge.
         items:
           type: string
-      reject_paths:
+      rejects_paths:
         type: array
         description: List of names of .rej files.
         items:

src/lando/api/legacy/workers/landing_worker.py

@@ -129,7 +129,6 @@ def notify_user_of_landing_failure(job: LandingJob):
             job.requester_email, job.landing_job_identifier, job.error, job.id
         )
 
-    # XXX Not covered by tests
     def process_merge_conflict(
         self,
         exception: PatchConflict,
@@ -138,7 +137,7 @@ def process_merge_conflict(
         revision_id: int,
     ) -> dict[str, Any]:
         """Extract and parse merge conflict data from exception into a usable format."""
-        failed_paths, reject_paths = self.extract_error_data(str(exception))
+        failed_paths, rejects_paths = self.extract_error_data(str(exception))
 
         # Find last commits to touch each failed path.
         failed_path_changesets = [
@@ -147,29 +146,28 @@ def process_merge_conflict(
 
         breakdown = {
             "revision_id": revision_id,
-            "content": None,
-            "reject_paths": None,
+            "rejects_paths": None,
         }
 
         breakdown["failed_paths"] = [
             {
-                "path": path[0],
-                "url": f"{repo.pull_path}/file/{path[1].decode('utf-8')}/{path[0]}",
-                "changeset_id": path[1].decode("utf-8"),
+                "path": path,
+                "url": f"{repo.pull_path}/file/{revision}/{path}",
+                "changeset_id": revision,
             }
-            for path in failed_path_changesets
+            for (path, revision) in failed_path_changesets
         ]
-        breakdown["reject_paths"] = {}
-        for path in reject_paths:
+        breakdown["rejects_paths"] = {}
+        for path in rejects_paths:
             reject = {"path": path}
             try:
-                with open(scm.REJECT_PATHS / repo.path[1:] / path, "r") as f:
+                with open(scm.get_rejects_path() / repo.path[1:] / path, "r") as f:
                     reject["content"] = f.read()
             except Exception as e:
                 logger.exception(e)
             # Use actual path of file to store reject data, by removing
             # `.rej` extension.
-            breakdown["reject_paths"][path[:-4]] = reject
+            breakdown["rejects_paths"][path[:-4]] = reject
         return breakdown
 
     @staticmethod
@@ -215,12 +213,12 @@ def extract_error_data(exception: str) -> tuple[list[str], list[str]]:
 
         # TODO: capture reason for patch failure, e.g. deleting non-existing file, or
         # adding a pre-existing file, etc...
-        reject_paths = rejs_re.findall(exception)
+        rejects_paths = rejs_re.findall(exception)
 
         # Collect all failed paths by removing `.rej` extension.
-        failed_paths = [path[:-4] for path in reject_paths]
+        failed_paths = [path[:-4] for path in rejects_paths]
 
-        return failed_paths, reject_paths
+        return failed_paths, rejects_paths
 
     def autoformat(
         self,

src/lando/api/tests/test_landings.py

@@ -456,6 +456,69 @@ def test_lose_push_race(
     assert job.status == LandingJobStatus.DEFERRED
 
 
+@pytest.mark.django_db
+def test_merge_conflict(
+    hg_server,
+    hg_clone,
+    treestatusdouble,
+    monkeypatch,
+    create_patch_revision,
+    normal_patch,
+    caplog,
+):
+    treestatusdouble.open_tree("mozilla-central")
+    repo = Repo.objects.create(
+        scm_type=SCM_TYPE_HG,
+        name="mozilla-central",
+        url=hg_server,
+        required_permission=SCM_LEVEL_3,
+        push_path=hg_server,
+        pull_path=hg_server,
+        system_path=hg_clone.strpath,
+    )
+    job_params = {
+        "id": 1234,
+        "status": LandingJobStatus.IN_PROGRESS,
+        "requester_email": "[email protected]",
+        "target_repo": repo,
+        "attempts": 1,
+    }
+    job = add_job_with_revisions(
+        [
+            create_patch_revision(1, patch=PATCH_FORMATTED_2),
+        ],
+        **job_params,
+    )
+
+    worker = LandingWorker(repos=Repo.objects.all(), sleep_seconds=0.01)
+
+    # We don't care about repo update in this test, however if we don't mock
+    # this, the test will fail since there is no celery instance.
+    monkeypatch.setattr(
+        "lando.api.legacy.workers.landing_worker.LandingWorker.phab_trigger_repo_update",
+        mock.MagicMock(),
+    )
+
+    assert worker.run_job(job)
+    assert job.status == LandingJobStatus.FAILED
+    assert "hunks FAILED" in caplog.text
+    assert job.error_breakdown, "No error breakdown added to job"
+    assert job.error_breakdown.get(
+        "rejects_paths"
+    ), "Empty or missing reject information in error breakdown"
+    failed_paths = [p["path"] for p in job.error_breakdown["failed_paths"]]
+    assert set(failed_paths) == set(
+        job.error_breakdown["rejects_paths"].keys()
+    ), "Mismatch between failed_paths and rejects_paths"
+    for fp in failed_paths:
+        assert job.error_breakdown["rejects_paths"][fp].get(
+            "path"
+        ), f"Empty or missing reject path for failed path {fp}"
+        assert job.error_breakdown["rejects_paths"][fp].get(
+            "content"
+        ), f"Empty or missing reject content for failed path {fp}"
+
+
 @pytest.mark.django_db
 def test_failed_landing_job_notification(
     hg_server,

src/lando/main/models/landing_job.py

@@ -76,7 +76,7 @@ def __str__(self):
     # Error details in a dictionary format, listing failed merges, etc...
     # E.g. {
     #    "failed_paths": [{"path": "...", "url": "..."}],
-    #    "reject_paths": [{"path": "...", "content": "..."}]
+    #    "rejects_paths": [{"path": "...", "content": "..."}]
     # }
     error_breakdown = models.JSONField(null=True, blank=True, default=dict)
 

src/lando/main/scm/abstract_scm.py

@@ -30,6 +30,16 @@ def scm_name(cls) -> str:
         """Return a _human-friendly_ string identifying the supported SCM (e.g.,
         `Mercurial`)."""
 
+    @classmethod
+    def get_rejects_path(cls) -> Path:
+        """Return the path where the SCM stores reject files.
+
+        We assume most SCMs just use the local directory, and provide a default
+        implementation. This however allows SCMs whose behaviour differs, such as
+        Mercurial, to provide a path to where the rejects are stored.
+        """
+        return Path(".")
+
     @abstractmethod
     def clone(self, source: str):
         """Clone a repository from a source.

src/lando/main/scm/hg.py

@@ -181,8 +181,8 @@ def scm_name(cls):
         """Return a _human-friendly_ string identifying the supported SCM."""
         return "Mercurial"
 
-    @property
-    def REJECTS_PATH(self) -> Path:
+    @classmethod
+    def get_rejects_path(cls) -> Path:
         """A Path where this SCM stores reject from a failed patch application."""
         return Path("/tmp/patch_rejects")
 
@@ -518,18 +518,18 @@ def _clean_and_close(self):
     def clean_repo(self, *, strip_non_public_commits: bool = True):
         """Clean the local working copy from all extraneous files."""
         # Reset rejects directory
-        if self.REJECTS_PATH.is_dir():
-            shutil.rmtree(self.REJECTS_PATH)
-        self.REJECTS_PATH.mkdir()
+        if self.get_rejects_path().is_dir():
+            shutil.rmtree(self.get_rejects_path())
+        self.get_rejects_path().mkdir()
 
         # Copy .rej files to a temporary folder.
         rejects = Path(f"{self.path}/").rglob("*.rej")
         for reject in rejects:
             os.makedirs(
-                self.REJECTS_PATH.joinpath(reject.parents[0].as_posix()[1:]),
+                self.get_rejects_path().joinpath(reject.parents[0].as_posix()[1:]),
                 exist_ok=True,
             )
-            shutil.copy(reject, self.REJECTS_PATH.joinpath(reject.as_posix()[1:]))
+            shutil.copy(reject, self.get_rejects_path().joinpath(reject.as_posix()[1:]))
 
         # Clean working directory.
         try:

src/lando/ui/jinja2/stack/partials/timeline.html

@@ -16,14 +16,14 @@
             </a>{% endfor %}
             </p>
             {% if transplant.error_breakdown %}
-            {% set reject_paths = transplant.error_breakdown.reject_paths %}
+            {% set rejects_paths = transplant.error_breakdown.rejects_paths %}
             <p>While applying <a href="{{ transplant.error_breakdown.revision_id|revision_url() }}">revision D{{ transplant.error_breakdown.revision_id }}</a> to {{ transplant.tree }}, the following files had conflicts:</p>
             <p>(Hint: try rebasing your changes on the latest commits from {{ transplant.tree }} and re-submitting.)</p>
             <div class="content">
                 <ul>
-                    {% for path in transplant.error_breakdown.failed_paths if path.path in reject_paths %}
+                    {% for path in transplant.error_breakdown.failed_paths if path.path in rejects_paths %}
                         <li><strong>{{ path.path }}</strong> @ <a href="{{ path.url }}">{{ path.changeset_id }}</a></li>
-                        {% set reject_lines = reject_paths[path.path].content.split("\n") %}
+                        {% set reject_lines = rejects_paths[path.path].content.split("\n") %}
                         {% if reject_lines.__len__() < 3 %}
                             <pre>{{ "\n".join(reject_lines) }}</pre>
                         {% else %}