From 7f7589c50ef627b9938a0c5f4881dc389745f9fe Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Thu, 28 Nov 2024 13:46:44 +0100
Subject: [PATCH] [FIX] runbot_merge: normalisation of patches before parsing

- Apparently if a user is on windows the ACE editor can swap out their
  line end from unix to windows. The patch parsers were predicated
  upon all patches being in unix mode (because git, and diff).

  Fixup both parsers to convert windows-style line end to unix before
  trying to parse the patch data. Also add a few fallbacks to limit
  the odds of an unhelpful `StopIteration` (though that might hide
  errors more than reveal them...)
- Make sure we support `format-patch --no-signature`, just requires
  using the correct partition direction: I assume I used `rpartition`
  as a form of micro-optimisation *but*

  - If the separator is not found the "patch body" ends up in the
    third parameter rather than the first, which makes the fallback
    difficult.
  - There doesn't seem to be anything preventing *multiple* signature
    separators in a message, and logically the first one should hold
    and the rest is all part of the signature.

  As a result, for both reasons we need to look *forwards* for the
  signature separator, not backwards. Hence `str.partition`.

Fixes #992
---
 runbot_merge/models/patcher.py      | 20 ++++++++++----------
 runbot_merge/tests/test_patching.py |  8 ++++++++
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/runbot_merge/models/patcher.py b/runbot_merge/models/patcher.py
index d7ad0dfd..868b6a75 100644
--- a/runbot_merge/models/patcher.py
+++ b/runbot_merge/models/patcher.py
@@ -53,20 +53,20 @@ def expect(line: str, starts_with: str, message: str) -> str:
 def parse_show(p: Patch) -> ParseResult:
     # headers are Author, Date or Author, AuthorDate, Commit, CommitDate
     # commit message is indented 4 spaces
-    lines = iter(p.patch.splitlines(keepends=True))
-    if not next(lines).startswith("commit "):
+    lines = (l + '\n' for l in p.patch.splitlines(keepends=False))
+    if not next(lines, '').startswith("commit "):
         raise ValidationError("Invalid patch")
     name, email = parseaddr(
-        expect(next(lines), "Author:", "Missing author")
+        expect(next(lines, ''), "Author:", "Missing author")
             .split(maxsplit=1)[1])
-    date: str = next(lines)
+    date: str = next(lines, '')
     header, date = date.split(maxsplit=1)
     author = (name, email, date)
     if header.startswith("Date:"):
         committer = author
     elif header.startswith("AuthorDate:"):
-        commit = expect(next(lines), "Commit:", "Missing committer")
-        commit_date = expect(next(lines), "CommitDate:", "Missing commit date")
+        commit = expect(next(lines, ''), "Commit:", "Missing committer")
+        commit_date = expect(next(lines, ''), "CommitDate:", "Missing commit date")
         name, email = parseaddr(commit.split(maxsplit=1)[1])
         committer = (name, email, commit_date.split(maxsplit=1)[1])
     else:
@@ -75,11 +75,11 @@ def parse_show(p: Patch) -> ParseResult:
             f"found {header}.\nOnly 'medium' and 'fuller' formats are supported")
 
     # skip possible extra headers before the message
-    while next(lines) != ' \n':
+    while next(lines, ' \n') != ' \n':
         continue
 
     body = []
-    while (l := next(lines)) != ' \n':
+    while (l := next(lines, ' \n')) != ' \n':
         body.append(l.removeprefix('    '))
 
     # remainder should be the patch
@@ -101,11 +101,11 @@ def parse_format_patch(p: Patch) -> ParseResult:
     msg = re.sub(r'^\[PATCH( \d+/\d+)?\] ', '', m['subject'])
     body, _, rest = m.get_payload().partition('---\n')
     if body:
-        msg += '\n\n' + body
+        msg += '\n\n' + body.replace('\r\n', '\n')
 
     # split off the signature, per RFC 3676 § 4.3.
     # leave the diffstat in as it *should* not confuse tooling?
-    patch, _, _ = rest.rpartition("-- \n")
+    patch, _, _ = rest.partition("-- \n")
     # git (diff, show, format-patch) adds command and index headers to every
     # file header, which patch(1) chokes on, strip them... but maybe this should
     # extract the udiff sections instead?
diff --git a/runbot_merge/tests/test_patching.py b/runbot_merge/tests/test_patching.py
index 2f2c8b64..a273e9cf 100644
--- a/runbot_merge/tests/test_patching.py
+++ b/runbot_merge/tests/test_patching.py
@@ -196,6 +196,14 @@ def test_apply_udiff(env, project, repo, users):
 @pytest.mark.parametrize('patch', [
     pytest.param(FORMAT_PATCH_XMO, id='xmo'),
     pytest.param(FORMAT_PATCH_MAT, id='mat'),
+    pytest.param(
+        FORMAT_PATCH_XMO.replace('\n', '\r\n'),
+        id='windows',
+    ),
+    pytest.param(
+        FORMAT_PATCH_XMO.rsplit('-- \n')[0],
+        id='no-signature',
+    )
 ])
 def test_apply_format_patch(env, project, repo, users, patch):
     p = env['runbot_merge.patch'].create({