[CHG] runbot_merge: convert rebase-ff staging to working locally

Implemented rebase (-ish?) by hand, following the model of the one
built on the github API, so we have all the information we need *and*
still don't need a local working copy.

Had to update *several* tests due to formatting changes from using the
CLI git (maybe that could be avoided by feeding the commit message
through stdin but w/e).

Also added a dupe of an existing helper for the formatting changes
bit, will have to remove it once all's done.

conftest.py

@@ -537,6 +537,9 @@ class Repo:
         self.hook = False
         repos.append(self)
 
+    def __repr__(self):
+        return f'<conftest.Repo {self.name}>'
+
     @property
     def owner(self):
         return self.name.split('/')[0]

forwardport/tests/test_simple.py

@@ -6,7 +6,7 @@ from datetime import datetime, timedelta
 
 import pytest
 
-from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of
+from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of, part_of2
 
 FMT = '%Y-%m-%d %H:%M:%S'
 FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT)
@@ -206,7 +206,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     old_b = prod.read_tree(b_head)
     head_b = prod.commit('b')
     assert head_b.message == message_template % pr1.number
-    assert prod.commit(head_b.parents[0]).message == part_of(f'p_0\n\nX-original-commit: {p_0_merged}', pr1, separator='\n')
+    assert prod.commit(head_b.parents[0]).message == part_of2(f'p_0\n\nX-original-commit: {p_0_merged}', pr1, separator='\n')
     b_tree = prod.read_tree(head_b)
     assert b_tree == {
         **old_b,
@@ -215,7 +215,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     old_c = prod.read_tree(c_head)
     head_c = prod.commit('c')
     assert head_c.message == message_template % pr2.number
-    assert prod.commit(head_c.parents[0]).message == part_of(f'p_0\n\nX-original-commit: {p_0_merged}', pr2, separator='\n')
+    assert prod.commit(head_c.parents[0]).message == part_of2(f'p_0\n\nX-original-commit: {p_0_merged}', pr2, separator='\n')
     c_tree = prod.read_tree(head_c)
     assert c_tree == {
         **old_c,

mergebot_test_utils/utils.py

@@ -8,7 +8,8 @@ MESSAGE_TEMPLATE = """{message}
 
 closes {repo}#{number}
 
-{headers}Signed-off-by: {name} <{email}>"""
+{headers}Signed-off-by: {name} <{email}>
+"""
 # target branch '-' source branch '-' base64 unique '-fw'
 REF_PATTERN = r'{target}-{source}-[a-zA-Z0-9_-]{{4}}-fw'
 
@@ -139,3 +140,6 @@ def part_of(label, pr_id, *, separator='\n\n'):
     """ Adds the "part-of" pseudo-header in the footer.
     """
     return f'{label}{separator}Part-of: {pr_id.display_name}'
+
+def part_of2(label, pr_id, *, separator='\n\n'):
+    return part_of(label, pr_id, separator=separator) + '\n'

runbot_merge/git.py

@@ -1,12 +1,14 @@
 import dataclasses
 import itertools
 import logging
+import os
 import pathlib
 import resource
 import subprocess
-from typing import Optional, TypeVar, Union, Generic
+from typing import Optional, TypeVar, Union, Generic, Sequence, Tuple, Dict
 
 from odoo.tools.appdirs import user_cache_dir
+from .github import MergeError, PrCommit
 
 _logger = logging.getLogger(__name__)
 
@@ -113,6 +115,70 @@ class Repo(Generic[T]):
         )
         return Repo(to)
 
+    def rebase(self, dest: str, commits: Sequence[PrCommit]) -> Tuple[str, Dict[str, str]]:
+        """Implements rebase by hand atop plumbing so:
+
+        - we can work without a working copy
+        - we can track individual commits (and store the mapping)
+
+        It looks like `--merge-base` is not sufficient for `merge-tree` to
+        correctly keep track of history, so it loses contents. Therefore
+        implement in two passes as in the github version.
+        """
+        repo = self.stdout().with_config(text=True, check=False)
+
+        logger = _logger.getChild('rebase')
+        logger.debug("rebasing %s on %s (reset=%s, commits=%s)",
+                     self._repo, dest, len(commits))
+        if not commits:
+            raise MergeError("PR has no commits")
+
+        new_trees = []
+        parent = dest
+        for original in commits:
+            if len(original['parents']) != 1:
+                raise MergeError(
+                    f"commits with multiple parents ({original['sha']}) can not be rebased, "
+                    "either fix the branch to remove merges or merge without "
+                    "rebasing")
+
+            new_trees.append(check(repo.merge_tree(parent, original['sha'])).stdout.strip())
+            parent = check(repo.commit_tree(
+                new_trees[-1],
+                '-p', parent,
+                '-p', original['sha'],
+                '-m', f'temp rebase {original["sha"]}',
+            )).stdout.strip()
+
+        mapping = {}
+        for original, t in zip(commits, new_trees):
+            authorship = check(repo.show('--no-patch', '--pretty="%an%n%ae%n%ai%n%cn%n%ce"', original['sha']))
+            author_name, author_email, author_date, committer_name, committer_email =\
+                authorship.stdout.splitlines()
+
+            c = check(repo.with_config(env={
+                **os.environ,
+                'GIT_AUTHOR_NAME': author_name,
+                'GIT_AUTHOR_EMAIL': author_email,
+                'GIT_AUTHOR_DATE': author_date,
+                'GIT_COMMITTER_NAME': committer_name,
+                'GIT_COMMITTER_EMAIL': committer_email,
+                'TZ': 'UTC',
+            }).commit_tree(t, p=dest, m=original['commit']['message']))\
+                .stdout.strip()
+
+            logger.debug('copied %s to %s (parent: %s)', original['sha'], c, dest)
+            dest = mapping[original['sha']] = c
+
+        return dest, mapping
+
+def check(p: subprocess.CompletedProcess) -> subprocess.CompletedProcess:
+    if not p.returncode:
+        return p
+
+    _logger.info("rebase failed at %s\nstdout:\n%s\nstderr:\n%s", p.args, p.stdout, p.stderr)
+    raise MergeError(p.stderr or 'merge conflict')
+
 
 @dataclasses.dataclass
 class GitCommand(Generic[T]):

runbot_merge/models/stagings_create.py

@@ -567,7 +567,16 @@ def stage_rebase_ff(pr: PullRequests, info: StagingSlice, target: str, commits:
     msg = pr._build_merge_message(commits[-1]['commit']['message'], related_prs=related_prs)
     commits[-1]['commit']['message'] = str(msg)
     add_self_references(pr, commits[:-1])
-    head, mapping = info.gh.rebase(pr.number, target, commits=commits)
+    head, mapping = info.repo.rebase(info.head, commits=commits)
+
+    # TODO: remove when we stop using tmp.
+    r = info.repo.with_config(text=True).check(False).push(
+        git.source_url(pr.repository, 'github'),
+        f'{head}:{target}'
+    )
+    if r.returncode:
+        raise exceptions.MergeError(pr, r.stderr)
+
     pr.commits_map = json.dumps({**mapping, '': head})
     return head
 

runbot_merge/tests/test_basic.py

@@ -10,7 +10,7 @@ import requests
 from lxml import html
 
 import odoo
-from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr, part_of
+from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr, part_of, part_of2
 
 
 @pytest.fixture
@@ -149,7 +149,7 @@ class TestCommitMessage:
 
         master = repo.commit('heads/master')
         assert master.message == "simple commit message\n\ncloses {repo.name}#1"\
-                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}\n"\
                                  .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_existing(self, env, repo, users, config):
@@ -174,7 +174,7 @@ class TestCommitMessage:
         master = repo.commit('heads/master')
         # closes #1 is already present, should not modify message
         assert master.message == "simple commit message that closes #1"\
-                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}\n"\
                                  .format(reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_other(self, env, repo, users, config):
@@ -199,7 +199,7 @@ class TestCommitMessage:
         master = repo.commit('heads/master')
         # closes on another repositoy, should modify the commit message
         assert master.message == "simple commit message that closes odoo/enterprise#1\n\ncloses {repo.name}#1"\
-                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}\n"\
                                  .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_wrong_number(self, env, repo, users, config):
@@ -224,7 +224,7 @@ class TestCommitMessage:
         master = repo.commit('heads/master')
         # closes on another repositoy, should modify the commit message
         assert master.message == "simple commit message that closes #11\n\ncloses {repo.name}#1"\
-                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}\n"\
                                  .format(repo=repo, reviewer=get_partner(env, users['reviewer']))
 
     def test_commit_delegate(self, env, repo, users, config):
@@ -254,7 +254,7 @@ class TestCommitMessage:
 
         master = repo.commit('heads/master')
         assert master.message == "simple commit message\n\ncloses {repo.name}#1"\
-                                 "\n\nSigned-off-by: {reviewer.formatted_email}"\
+                                 "\n\nSigned-off-by: {reviewer.formatted_email}\n"\
                                  .format(repo=repo, reviewer=get_partner(env, users['other']))
 
     def test_commit_coauthored(self, env, repo, users, config):
@@ -292,7 +292,8 @@ Fixes a thing
 closes {repo.name}#1
 
 Signed-off-by: {reviewer.formatted_email}
-Co-authored-by: Bob <bob@example.com>""".format(
+Co-authored-by: Bob <bob@example.com>
+""".format(
             repo=repo,
             reviewer=get_partner(env, users['reviewer'])
         )
@@ -1628,8 +1629,8 @@ commits, I need to know how to merge it:
         # then compare to the dag version of the right graph
         nm2 = node('M2', node('M1', node('M0')))
         reviewer = get_partner(env, users["reviewer"]).formatted_email
-        nb1 = node(f'B1\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}',
+        nb1 = node(f'B1\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}\n',
-                   node(part_of('B0', pr_id), nm2))
+                   node(part_of2('B0', pr_id), nm2))
         assert staging == nb1
 
         with repo:
@@ -1718,7 +1719,7 @@ commits, I need to know how to merge it:
         c0 = node('C0', m)
         reviewer = get_partner(env, users["reviewer"]).formatted_email
         expected = node('gibberish\n\nblahblah\n\ncloses {}#{}'
-                        '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0)
+                        '\n\nSigned-off-by: {}\n'.format(repo.name, prx.number, reviewer), m, c0)
         assert log_to_node(repo.log('heads/master')), expected
         pr = env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
@@ -1760,7 +1761,7 @@ commits, I need to know how to merge it:
         c0 = node('C0', m)
         reviewer = get_partner(env, users["reviewer"]).formatted_email
         expected = node('gibberish\n\ncloses {}#{}'
-                        '\n\nSigned-off-by: {}'.format(repo.name, prx.number, reviewer), m, c0)
+                        '\n\nSigned-off-by: {}\n'.format(repo.name, prx.number, reviewer), m, c0)
         assert log_to_node(repo.log('heads/master')), expected
 
     @pytest.mark.parametrize('separator', [
@@ -1790,15 +1791,14 @@ commits, I need to know how to merge it:
         env.run_crons()
 
         head = repo.commit('heads/master')
-        assert head.message == textwrap.dedent(f"""\
+        assert head.message == f"""\
-        title
+title
 
-        first
+first
 
-        closes {repo.name}#{pr.number}
+closes {repo.name}#{pr.number}
 
-        Signed-off-by: {reviewer}
+Signed-off-by: {reviewer}""", "should not contain the content which follows the thematic break"
-        """).strip(), "should not contain the content which follows the thematic break"
 
     def test_pr_message_setex_title(self, repo, env, users, config):
         """ should not break on a proper SETEX-style title """
@@ -1830,21 +1830,20 @@ removed
         env.run_crons()
 
         head = repo.commit('heads/master')
-        assert head.message == textwrap.dedent(f"""\
+        assert head.message == f"""\
-        title
+title
 
-        Title
+Title
-        ---
+---
-        This is some text
+This is some text
 
-        Title 2
+Title 2
-        -------
+-------
-        This is more text
+This is more text
 
-        closes {repo.name}#{pr.number}
+closes {repo.name}#{pr.number}
 
-        Signed-off-by: {reviewer}
+Signed-off-by: {reviewer}""", "should not break the SETEX titles"
-        """).strip(), "should not break the SETEX titles"
 
     def test_rebase_no_edit(self, repo, env, users, config):
         """ Only the merge messages should be de-breaked
@@ -1867,17 +1866,17 @@ removed
         env.run_crons()
 
         head = repo.commit('heads/master')
-        assert head.message == textwrap.dedent(f"""\
+        assert head.message == f"""\
-        Commit
+Commit
 
-        first
+first
-        ***
+***
-        second
+second
 
-        closes {repo.name}#{pr.number}
+closes {repo.name}#{pr.number}
 
-        Signed-off-by: {reviewer}
+Signed-off-by: {reviewer}
-        """).strip(), "squashed / rebased messages should not be stripped"
+""", "squashed / rebased messages should not be stripped"
 
     def test_title_no_edit(self, repo, env, users, config):
         """The first line of a commit message should not be taken in account for
@@ -1909,13 +1908,15 @@ thing: thong
 
 closes {pr_id.display_name}
 
-Signed-off-by: {reviewer}"""
+Signed-off-by: {reviewer}
+"""
         assert repo.commit(staging_head.parents[0]).message == f"""\
 Some: thing
 
 is odd
 
-Part-of: {pr_id.display_name}"""
+Part-of: {pr_id.display_name}
+"""
 
     def test_pr_mergehead(self, repo, env, config):
         """ if the head of the PR is a merge commit and one of the parents is
@@ -2741,8 +2742,8 @@ class TestBatching(object):
 
         staging = log_to_node(log)
         reviewer = get_partner(env, users["reviewer"]).formatted_email
-        expected = node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr2.number, reviewer),
+        expected = node('commit_PR2_00\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, pr2.number, reviewer),
-             node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.number, reviewer),
+             node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}\n'.format(repo.name, pr1.number, reviewer),
                   node('initial')))
         assert staging == expected
 

runbot_merge/tests/test_multirepo.py

@@ -435,7 +435,7 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config):
         )
     env.run_crons()
 
-    s2 = to_pr(env, pr2a) | to_pr(env, pr2b)
+    pr2a_id, pr2b_id = s2 = to_pr(env, pr2a) | to_pr(env, pr2b)
     st = env['runbot_merge.stagings'].search([])
     assert set(st.batch_ids.prs.ids) == set(s2.ids)
 
@@ -453,12 +453,14 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config):
         c['commit']['message']
         for c in repo_a.log('heads/staging.master')
     ] == [
-        """commit_do-b-thing_00
+        f"""\
+commit_do-b-thing_00
 
-closes %s
+closes {pr2a_id.display_name}
 
-Related: %s
+Related: {pr2b_id.display_name}
-Signed-off-by: %s""" % (s2[0].display_name, s2[1].display_name, reviewer),
+Signed-off-by: {reviewer}
+""",
         'initial'
     ], "dummy commit + squash-merged PR commit + root commit"