diff --git a/conftest.py b/conftest.py index a244f2af..3d936e56 100644 --- a/conftest.py +++ b/conftest.py @@ -537,6 +537,9 @@ class Repo: self.hook = False repos.append(self) + def __repr__(self): + return f'' + @property def owner(self): return self.name.split('/')[0] diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 5f5e21b8..8bdb7779 100644 --- a/forwardport/tests/test_simple.py +++ b/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, diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index b87a2345..42b20a9d 100644 --- a/mergebot_test_utils/utils.py +++ b/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' diff --git a/runbot_merge/git.py b/runbot_merge/git.py index 7fd109c0..a06a293d 100644 --- a/runbot_merge/git.py +++ b/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]): diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index 87fc3738..432a16b1 100644 --- a/runbot_merge/models/stagings_create.py +++ b/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 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 0b20d476..f7378f49 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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 """.format( +Co-authored-by: Bob +""".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}', - node(part_of('B0', pr_id), nm2)) + nb1 = node(f'B1\n\ncloses {pr_id.display_name}\n\nSigned-off-by: {reviewer}\n', + 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"""\ - title + assert head.message == f"""\ +title - first +first - closes {repo.name}#{pr.number} +closes {repo.name}#{pr.number} - Signed-off-by: {reviewer} - """).strip(), "should not contain the content which follows the thematic break" +Signed-off-by: {reviewer}""", "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"""\ - title + assert head.message == f"""\ +title - Title - --- - This is some text +Title +--- +This is some text - Title 2 - ------- - This is more text +Title 2 +------- +This is more text - closes {repo.name}#{pr.number} +closes {repo.name}#{pr.number} - Signed-off-by: {reviewer} - """).strip(), "should not break the SETEX titles" +Signed-off-by: {reviewer}""", "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"""\ - Commit + assert head.message == f"""\ +Commit - first - *** - second +first +*** +second - closes {repo.name}#{pr.number} +closes {repo.name}#{pr.number} - Signed-off-by: {reviewer} - """).strip(), "squashed / rebased messages should not be stripped" +Signed-off-by: {reviewer} +""", "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), - node('commit_PR1_00\n\ncloses {}#{}\n\nSigned-off-by: {}'.format(repo.name, pr1.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: {}\n'.format(repo.name, pr1.number, reviewer), node('initial'))) assert staging == expected diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 57698426..908b7db4 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/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 -Signed-off-by: %s""" % (s2[0].display_name, s2[1].display_name, reviewer), +Related: {pr2b_id.display_name} +Signed-off-by: {reviewer} +""", 'initial' ], "dummy commit + squash-merged PR commit + root commit"