Repository: smarkets/marge-bot

Issue:
Merge instead of rebase
Would it be possible to add an option to use a git merge instead of a git rebase ?

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b9ef871..2081ca5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,5 @@
+  * 0.5.1:
+    - Sleep even less between polling for MRs #75.
   * 0.5.0:
     - Added "default -> config file -> env var -> args" way to configure marge-bot #71
   * 0.4.1:
diff --git a/marge/app.py b/marge/app.py
index 32f8db5..6fdee3b 100644
--- a/marge/app.py
+++ b/marge/app.py
@@ -100,7 +100,16 @@ def _parse_config(args):
         metavar='INTERVAL[,..]',
         help='Time(s) during which no merging is to take place, e.g. "Friday 1pm - Monday 9am".\n',
     )
-    parser.add_argument(
+    merge_group = parser.add_mutually_exclusive_group(required=False)
+    merge_group.add_argument(
+        '--use-merge-strategy',
+        action='store_true',
+        help=(
+            'Use git merge instead of git rebase\n'
+            'Enable if you use a workflow based on merge-commits and not linear history\n'
+        ),
+    )
+    merge_group.add_argument(
         '--add-tested',
         action='store_true',
         help='Add "Tested: marge-bot <$MR_URL>" for the final commit on branch after it passed CI.\n',
@@ -214,6 +223,7 @@ def main(args=sys.argv[1:]):
                 reapprove=options.impersonate_approvers,
                 embargo=options.embargo,
                 ci_timeout=options.ci_timeout,
+                use_merge_strategy=options.use_merge_strategy,
             )
         )
 
diff --git a/marge/bot.py b/marge/bot.py
index 7b32544..0c52e75 100644
--- a/marge/bot.py
+++ b/marge/bot.py
@@ -47,6 +47,8 @@ class Bot(object):
         return self._api
 
     def _run(self, repo_manager):
+        time_to_sleep_between_projects_in_secs = 1
+        min_time_to_sleep_after_iterating_all_projects_in_secs = 30
         while True:
             log.info('Finding out my current projects...')
             my_projects = Project.fetch_all_mine(self._api)
@@ -104,12 +106,15 @@ class Bot(object):
                         options=self._config.merge_opts,
                     )
                     merge_job.execute()
-                    time_to_sleep_in_secs = 5
                 else:
                     log.info('Nothing to merge at this point...')
-                    time_to_sleep_in_secs = 30
-                log.info('Sleeping for %s seconds...', time_to_sleep_in_secs)
-                time.sleep(time_to_sleep_in_secs)
+                time.sleep(time_to_sleep_between_projects_in_secs)
+            big_sleep = max(0,
+                            min_time_to_sleep_after_iterating_all_projects_in_secs -
+                            time_to_sleep_between_projects_in_secs * len(filtered_projects))
+            log.info('Sleeping for %s seconds...', big_sleep)
+            time.sleep(big_sleep)
+
 
 
 
diff --git a/marge/git.py b/marge/git.py
index 3c3c453..fa7c68a 100644
--- a/marge/git.py
+++ b/marge/git.py
@@ -59,6 +59,17 @@ class Repo(namedtuple('Repo', 'remote_url local_path ssh_key_file timeout')):
             raise
         return self.get_commit_hash()
 
+    def merge(self, source_branch, target_branch, source_repo_url=None):
+        """Merge `target_branch` into `source_branch` and return the new HEAD commit id.
+
+        By default `source_branch` and `target_branch` are assumed to reside in the same
+        repo as `self`. However, if `source_repo_url` is passed and not `None`,
+        `source_branch` is taken from there.
+
+        Throws a `GitError` if the merge fails. Will also try to --abort it.
+        """
+        return self._fuse_branch('merge', source_branch, target_branch, source_repo_url)
+
     def rebase(self, branch, new_base, source_repo_url=None):
         """Rebase `new_base` into `branch` and return the new HEAD commit id.
 
@@ -68,7 +79,10 @@ class Repo(namedtuple('Repo', 'remote_url local_path ssh_key_file timeout')):
 
         Throws a `GitError` if the rebase fails. Will also try to --abort it.
         """
-        assert source_repo_url or branch != new_base, branch
+        return self._fuse_branch('rebase', branch, new_base, source_repo_url)
+
+    def _fuse_branch(self, strategy, branch, target_branch, source_repo_url=None):
+        assert source_repo_url or branch != target_branch, branch
 
         self.git('fetch', 'origin')
         if source_repo_url:
@@ -84,10 +98,10 @@ class Repo(namedtuple('Repo', 'remote_url local_path ssh_key_file timeout')):
             self.git('checkout', '-B', branch, 'origin/' + branch, '--')
 
         try:
-            self.git('rebase', 'origin/' + new_base)
+            self.git(strategy, 'origin/' + target_branch)
         except GitError:
-            log.warning('rebase failed, doing an --abort')
-            self.git('rebase', '--abort')
+            log.warning(strategy + ' failed, doing an --abort')
+            self.git(strategy, '--abort')
             raise
         return self.get_commit_hash()
 
diff --git a/marge/job.py b/marge/job.py
index 85840e2..82d80f2 100644
--- a/marge/job.py
+++ b/marge/job.py
@@ -52,7 +52,7 @@ class MergeJob(object):
                 return
 
             approvals = merge_request.fetch_approvals()
-            self.rebase_and_accept(approvals)
+            self.update_merge_request_and_accept(approvals)
             log.info('Successfully merged !%s.', merge_request.info['iid'])
         except CannotMerge as err:
             message = "I couldn't merge this branch: %s" % err.reason
@@ -69,12 +69,12 @@ class MergeJob(object):
             self.unassign_from_mr(merge_request)
             raise
 
-    def rebase_and_accept(self, approvals):
+    def update_merge_request_and_accept(self, approvals):
         api = self._api
         merge_request = self._merge_request
-        rebased_into_up_to_date_target_branch = False
+        updated_into_up_to_date_target_branch = False
 
-        while not rebased_into_up_to_date_target_branch:
+        while not updated_into_up_to_date_target_branch:
             if merge_request.work_in_progress:
                 raise CannotMerge("Sorry, I can't merge requests marked as Work-In-Progress!")
             if merge_request.squash and self.opts.requests_commit_tagging:
@@ -106,8 +106,8 @@ class MergeJob(object):
                 else None
             )
             source_repo_url = None if source_project is self._project else source_project.ssh_url_to_repo
-            # NB. this will be a no-op if there is nothing to rebase/rewrite
-            target_sha, _rebased_sha, actual_sha = push_rebased_and_rewritten_version(
+            # NB. this will be a no-op if there is nothing to update/rewrite
+            target_sha, _updated_sha, actual_sha = update_from_target_branch_and_push(
                 repo=self.repo,
                 source_branch=merge_request.source_branch,
                 target_branch=merge_request.target_branch,
@@ -115,6 +115,7 @@ class MergeJob(object):
                 reviewers=reviewers,
                 tested_by=tested_by,
                 part_of=part_of,
+                use_merge_strategy=self.opts.use_merge_strategy,
             )
             log.info('Commit id to merge %r (into: %r)', actual_sha, target_sha)
             time.sleep(5)
@@ -144,7 +145,7 @@ class MergeJob(object):
                 merge_request.accept(remove_branch=True, sha=actual_sha)
             except gitlab.NotAcceptable as err:
                 new_target_sha = Commit.last_on_branch(self._project.id, merge_request.target_branch, api).id
-                # target_branch has moved under us since we rebased, just try again
+                # target_branch has moved under us since we updated, just try again
                 if new_target_sha != target_sha:
                     log.info('Someone was naughty and by-passed marge')
                     merge_request.comment(
@@ -166,7 +167,7 @@ class MergeJob(object):
                     # someone must have hit "merge when build succeeds" and we lost the race,
                     # the branch is gone and we got a 404. Anyway, our job here is done.
                     # (see #33)
-                    rebased_into_up_to_date_target_branch = True
+                    updated_into_up_to_date_target_branch = True
                 else:
                     log.warning('For the record, merge request state is %r', merge_request.state)
                     raise
@@ -188,7 +189,7 @@ class MergeJob(object):
                     # We are not covering any observed behaviour here, but if at this
                     # point the request is merged, our job is done, so no need to complain
                     log.info('Merge request is already merged, someone was faster!')
-                    rebased_into_up_to_date_target_branch = True
+                    updated_into_up_to_date_target_branch = True
                 else:
                     raise CannotMerge("Gitlab refused to merge this request and I don't know why!")
             except gitlab.ApiError:
@@ -196,7 +197,7 @@ class MergeJob(object):
                 raise CannotMerge('had some issue with gitlab, check my logs...')
             else:
                 self.wait_for_branch_to_be_merged()
-                rebased_into_up_to_date_target_branch = True
+                updated_into_up_to_date_target_branch = True
 
     def wait_for_ci_to_pass(self, source_project_id, commit_sha):
         api = self._api
@@ -253,7 +254,8 @@ class MergeJob(object):
         now = datetime.utcnow()
         return self.opts.embargo.covers(now)
 
-def push_rebased_and_rewritten_version(
+
+def update_from_target_branch_and_push(
         *,
         repo,
         source_branch,
@@ -262,15 +264,17 @@ def push_rebased_and_rewritten_version(
         reviewers=None,
         tested_by=None,
         part_of=None,
+        use_merge_strategy=False,
 ):
-    """Rebase `target_branch` into `source_branch`, optionally add trailers and push.
+    """Updates `target_branch` with commits from `source_branch`, optionally add trailers and push.
+    The update strategy can either be rebase or merge. The default is rebase.
 
     Parameters
     ----------
     source_branch
-       The branch we want to rebase to.
+       The branch we want to update.
     target_branch
-       The branch we want to rebase from.
+       The branch we want to get updates from.
     source_repo_url
        The url of the repo we want to push the changes to (or `None` if it's the
        same repo for both `source_branch` and `target_branch`).
@@ -282,25 +286,28 @@ def push_rebased_and_rewritten_version(
     tested_by
        A list like ``["User Name <u.name@invalid.com>", ...]`` or `None`. ``None`` means
        existing Tested-by lines will be left alone, otherwise they will be replaced.
+       Ignored if using the merge strategy.
     part_of
        A string with likely a link to the merge request this commit is part-of, or ``None``.
-
+    use_merge_strategy
+       Updates `target_branch` using merge instead of rebase.
     Returns
     -------
-    (sha_of_target_branch, sha_after_rebase, sha_after_rewrite)
+    (sha_of_target_branch, sha_after_update, sha_after_rewrite)
     """
     assert source_repo_url != repo.remote_url
     if source_repo_url is None and source_branch == target_branch:
         raise CannotMerge('source and target branch seem to coincide!')
 
-    branch_rebased = branch_rewritten = changes_pushed = False
+    branch_updated = branch_rewritten = changes_pushed = False
     try:
-        rewritten_sha = rebased_sha = repo.rebase(
+        fuse = repo.merge if use_merge_strategy else repo.rebase
+        rewritten_sha = updated_sha = fuse(
             branch=source_branch,
             new_base=target_branch,
             source_repo_url=source_repo_url
         )
-        branch_rebased = True
+        branch_updated = True
         if reviewers is not None:
             rewritten_sha = repo.tag_with_trailer(
                 trailer_name='Reviewed-by',
@@ -308,7 +315,7 @@ def push_rebased_and_rewritten_version(
                 branch=source_branch,
                 start_commit='origin/' + target_branch,
             )
-        if tested_by is not None:
+        if tested_by is not None and not use_merge_strategy:
             rewritten_sha = repo.tag_with_trailer(
                 trailer_name='Tested-by',
                 trailer_values=tested_by,
@@ -326,17 +333,20 @@ def push_rebased_and_rewritten_version(
         repo.push_force(source_branch, source_repo_url)
         changes_pushed = True
     except git.GitError:
-        if not branch_rebased:
+        if not branch_updated:
             raise CannotMerge('got conflicts while rebasing, your problem now...')
         if not branch_rewritten:
             raise CannotMerge('failed on filter-branch; check my logs!')
         if not changes_pushed:
-            raise CannotMerge('failed to push rebased changes, check my logs!')
+            if use_merge_strategy:
+                raise CannotMerge('failed to push merged changes, check my logs!')
+            else:
+                raise CannotMerge('failed to push rebased changes, check my logs!')
 
         raise
     else:
         target_sha = repo.get_commit_hash('origin/' + target_branch)
-        return target_sha, rebased_sha, rewritten_sha
+        return target_sha, updated_sha, rewritten_sha
     finally:
         # A failure to clean up probably means something is fucked with the git repo
         # and likely explains any previous failure, so it will better to just
@@ -346,6 +356,7 @@ def push_rebased_and_rewritten_version(
         else:
             assert source_repo_url is not None
 
+
 def _get_reviewer_names_and_emails(approvals, api):
     """Return a list ['A. Prover <a.prover@example.com', ...]` for `merge_request.`"""
 
@@ -360,6 +371,7 @@ _job_options = [
     'reapprove',
     'embargo',
     'ci_timeout',
+    'use_merge_strategy',
 ]
 
 class MergeJobOptions(namedtuple('MergeJobOptions', _job_options)):
@@ -373,7 +385,7 @@ class MergeJobOptions(namedtuple('MergeJobOptions', _job_options)):
     def default(
             cls, *,
             add_tested=False, add_part_of=False, add_reviewers=False, reapprove=False,
-            embargo=None, ci_timeout=None,
+            embargo=None, ci_timeout=None, use_merge_strategy=False
     ):
         embargo = embargo or IntervalUnion.empty()
         ci_timeout = ci_timeout or timedelta(minutes=15)
@@ -384,6 +396,7 @@ class MergeJobOptions(namedtuple('MergeJobOptions', _job_options)):
             reapprove=reapprove,
             embargo=embargo,
             ci_timeout=ci_timeout,
+            use_merge_strategy=use_merge_strategy,
         )
 
 
diff --git a/version b/version
index 8f0916f..4b9fcbe 100644
--- a/version
+++ b/version
@@ -1,1 +1,1 @@
-0.5.0
+0.5.1


Test patch:
diff --git a/tests/test_app.py b/tests/test_app.py
index d47b36e..d700fc7 100644
--- a/tests/test_app.py
+++ b/tests/test_app.py
@@ -101,12 +101,24 @@ def test_embargo():
                embargo=interval.IntervalUnion.from_human('Fri 1pm-Mon 7am'),
             )
 
+def test_use_merge_strategy():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with main('--use-merge-strategy') as bot:
+            assert bot.config.merge_opts != job.MergeJobOptions.default()
+            assert bot.config.merge_opts == job.MergeJobOptions.default(use_merge_strategy=True)
+
 def test_add_tested():
     with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
         with main('--add-tested') as bot:
             assert bot.config.merge_opts != job.MergeJobOptions.default()
             assert bot.config.merge_opts == job.MergeJobOptions.default(add_tested=True)
 
+def test_use_merge_strategy_and_add_tested_are_mutualy_exclusive():
+    with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
+        with pytest.raises(SystemExit):
+            with main('--use-merge-strategy --add-tested') as bot:
+                pass
+
 def test_add_part_of():
     with env(MARGE_AUTH_TOKEN="NON-ADMIN-TOKEN", MARGE_SSH_KEY="KEY", MARGE_GITLAB_URL='http://foo.com'):
         with main('--add-part-of') as bot:
diff --git a/tests/test_git.py b/tests/test_git.py
index d25a773..4569d03 100644
--- a/tests/test_git.py
+++ b/tests/test_git.py
@@ -44,6 +44,16 @@ class TestRepo(object):
             'git -C /tmp/local/path rev-parse HEAD'
         ]
 
+    def test_merge_success(self, mocked_run):
+        self.repo.merge('feature_branch', 'master_of_the_universe')
+
+        assert get_calls(mocked_run) == [
+            'git -C /tmp/local/path fetch origin',
+            'git -C /tmp/local/path checkout -B feature_branch origin/feature_branch --',
+            'git -C /tmp/local/path merge origin/master_of_the_universe',
+            'git -C /tmp/local/path rev-parse HEAD'
+        ]
+
     def test_reviewer_tagging_success(self, mocked_run):
         self.repo.tag_with_trailer(
             trailer_name='Reviewed-by',
@@ -89,6 +99,12 @@ class TestRepo(object):
 
         assert get_calls(mocked_run) == []
 
+    def test_merge_same_branch(self, mocked_run):
+        with pytest.raises(AssertionError):
+            self.repo.merge('branch', 'branch')
+
+        assert get_calls(mocked_run) == []
+
     def test_remove_branch(self, mocked_run):
         self.repo.remove_branch('some_branch')
         assert get_calls(mocked_run) == [
diff --git a/tests/test_job.py b/tests/test_job.py
index 1cb5868..3e7b790 100644
--- a/tests/test_job.py
+++ b/tests/test_job.py
@@ -134,10 +134,10 @@ class MockLab(object):
             "I'm broken on the inside, please somebody fix me... :cry:"
         )
 
-    def push_rebased(self, *args, **kwargs):
+    def push_updated(self, *args, **kwargs):
         self.api.state = 'pushed'
-        rebased_sha = 'deadbeef'
-        return self.initial_master_sha, rebased_sha, self.rewritten_sha
+        updated_sha = 'deadbeef'
+        return self.initial_master_sha, updated_sha, self.rewritten_sha
 
     @contextlib.contextmanager
     def expected_failure(self, message):
@@ -159,7 +159,7 @@ class MockLab(object):
         assert error_note in self.api.notes
 
 @patch('time.sleep')
-class TestRebaseAndAccept(object):
+class TestUpdateAndAccept(object):
     def setup_method(self, _method):
         self.mocklab = MockLab()
         self.api = self.mocklab.api
@@ -184,7 +184,7 @@ class TestRebaseAndAccept(object):
 
     def test_succeeds_first_time(self, time_sleep):
         api, mocklab = self.api, self.mocklab
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False))
             job.execute()
 
@@ -199,7 +199,7 @@ class TestRebaseAndAccept(object):
            Ok({'commit': _commit(id=new_branch_head_sha, status='success')}),
            from_state='pushed', to_state='pushed_but_head_changed'
         )
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             with mocklab.expected_failure("Someone pushed to branch while we were trying to merge"):
                 job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False))
                 job.execute()
@@ -244,7 +244,7 @@ class TestRebaseAndAccept(object):
             api.state = 'pushed'
             yield moved_master_sha, 'deadbeef', mocklab.rewritten_sha
 
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=push_effects()):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=push_effects()):
             job = self.make_job(marge.job.MergeJobOptions.default(add_tested=True, add_reviewers=False))
             job.execute()
 
@@ -266,7 +266,7 @@ class TestRebaseAndAccept(object):
             dict(mocklab.merge_request_info, state='merged'),
             from_state='someone_else_merged',
         )
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             job = self.make_job()
             job.execute()
         assert api.state == 'someone_else_merged'
@@ -288,7 +288,7 @@ class TestRebaseAndAccept(object):
             from_state='now_is_wip',
         )
         message = 'The request was marked as WIP as I was processing it (maybe a WIP commit?)'
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             with mocklab.expected_failure(message):
                 job = self.make_job()
                 job.execute()
@@ -314,7 +314,7 @@ class TestRebaseAndAccept(object):
             'GitLab refused to merge this branch. I suspect that a Push Rule or a git-hook '
             'is rejecting my commits; maybe my email needs to be white-listed?'
         )
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             with mocklab.expected_failure(message):
                 job = self.make_job()
                 job.execute()
@@ -337,7 +337,7 @@ class TestRebaseAndAccept(object):
             from_state='oops_someone_closed_it',
         )
         message = 'Someone closed the merge request while I was attempting to merge it.'
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             with mocklab.expected_failure(message):
                 job = self.make_job()
                 job.execute()
@@ -356,7 +356,7 @@ class TestRebaseAndAccept(object):
             from_state='passed', to_state='rejected_for_misterious_reasons',
         )
         message = "Gitlab refused to merge this request and I don't know why!"
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             with mocklab.expected_failure(message):
                 job = self.make_job()
                 job.execute()
@@ -392,7 +392,7 @@ class TestRebaseAndAccept(object):
 
             assert api.state == 'initial'
 
-        with patch('marge.job.push_rebased_and_rewritten_version', side_effect=mocklab.push_rebased):
+        with patch('marge.job.update_from_target_branch_and_push', side_effect=mocklab.push_updated):
             job = self.make_job()
             job.execute()
         assert api.state == 'merged'
@@ -407,6 +407,7 @@ class TestMergeJobOptions(object):
             reapprove=False,
             embargo=marge.interval.IntervalUnion.empty(),
             ci_timeout=timedelta(minutes=15),
+            use_merge_strategy=False,
         )
 
     def test_default_ci_time(self):