Optimistic batching for batch merging

Fix for issue #164.

Previously we added trailers to each MR and
merged the constituent MRs of a batch MR to
the target branch.
Now we will now optimistically add the trailers
to each MR and rebase on the batch branch. And
when the batch branch passes CI we merge it
which will automatically merge each constituent
branch.

Author: David Szervanszky

marge/batch_job.py

@@ -112,7 +112,6 @@ def accept_mr(
         self,
         merge_request,
         expected_remote_target_branch_sha,
-        source_repo_url=None,
     ):
         log.info('Fusing MR !%s', merge_request.iid)
         approvals = merge_request.fetch_approvals()
@@ -122,21 +121,6 @@ def accept_mr(
         if new_target_sha != expected_remote_target_branch_sha:
             raise CannotBatch('Someone was naughty and by-passed marge')
 
-        # FIXME: we should only add tested-by for the last MR in the batch
-        _, _, actual_sha = self.update_from_target_branch_and_push(
-            merge_request,
-            source_repo_url=source_repo_url,
-        )
-
-        sha_now = Commit.last_on_branch(
-            merge_request.source_project_id, merge_request.source_branch, self._api,
-        ).id
-        # Make sure no-one managed to race and push to the branch in the
-        # meantime, because we're about to impersonate the approvers, and
-        # we don't want to approve unreviewed commits
-        if sha_now != actual_sha:
-            raise CannotMerge('Someone pushed to branch while we were trying to merge')
-
         # As we're not using the API to merge the MR, we don't strictly need to reapprove it. However,
         # it's a little weird to look at the merged MR to find it has no approvals, so let's do it anyway.
         self.maybe_reapprove(merge_request, approvals)
@@ -198,6 +182,9 @@ def execute(self):
                     merge_request.source_branch,
                     '%s/%s' % (merge_request_remote, merge_request.source_branch),
                 )
+                # Apply the trailers before running the batch MR
+                self.add_trailers(merge_request)
+                self.push_force_to_mr(merge_request, True, source_repo_url, skip_ci=True)
 
                 # Update <source_branch> on latest <batch> branch so it contains previous MRs
                 self.fuse(
@@ -207,6 +194,9 @@ def execute(self):
                     local=True,
                 )
 
+                # Ensure that individual branch commit SHA matches matches that of its equivalent in batch MR
+                self.push_force_to_mr(merge_request, True, source_repo_url, skip_ci=True)
+
                 # Update <batch> branch with MR changes
                 self._repo.fast_forward(
                     BatchMergeJob.BATCH_BRANCH_NAME,
@@ -225,36 +215,34 @@ def execute(self):
                 working_merge_requests.append(merge_request)
         if len(working_merge_requests) <= 1:
             raise CannotBatch('not enough ready merge requests')
-        if self._project.only_allow_merge_if_pipeline_succeeds:
-            # This switches git to <batch> branch
-            self.push_batch()
-            batch_mr = self.create_batch_mr(
-                target_branch=target_branch,
-            )
+        # This switches git to <batch> branch
+        self.push_batch()
+        batch_mr = self.create_batch_mr(
+            target_branch=target_branch,
+        )
+        for merge_request in working_merge_requests:
+            merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid))
+        try:
+            self.wait_for_ci_to_pass(batch_mr)
+        except CannotMerge as err:
             for merge_request in working_merge_requests:
-                merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid))
-            try:
-                self.wait_for_ci_to_pass(batch_mr)
-            except CannotMerge as err:
-                for merge_request in working_merge_requests:
-                    merge_request.comment(
-                        'Batch MR !{batch_mr_iid} failed: {error} I will retry later...'.format(
-                            batch_mr_iid=batch_mr.iid,
-                            error=err.reason,
-                        ),
-                    )
-                raise CannotBatch(err.reason) from err
+                merge_request.comment(
+                    'Batch MR !{batch_mr_iid} failed: {error} I will retry later...'.format(
+                        batch_mr_iid=batch_mr.iid,
+                        error=err.reason,
+                    ),
+                )
+            raise CannotBatch(err.reason) from err
         for merge_request in working_merge_requests:
             try:
                 # FIXME: this should probably be part of the merge request
                 _, source_repo_url, merge_request_remote = self.fetch_source_project(merge_request)
-                self.ensure_mr_not_changed(merge_request)
                 self.ensure_mergeable_mr(merge_request)
-                remote_target_branch_sha = self.accept_mr(
-                    merge_request,
-                    remote_target_branch_sha,
-                    source_repo_url=source_repo_url,
-                )
+                if merge_request == working_merge_requests[-1]:
+                    self.accept_mr(
+                        batch_mr,
+                        remote_target_branch_sha,
+                    )
             except CannotBatch as err:
                 merge_request.comment(
                     "I couldn't merge this branch: {error} I will retry later...".format(

marge/git.py

@@ -131,7 +131,7 @@ def remove_branch(self, branch, *, new_current_branch='master'):
     def checkout_branch(self, branch, start_point=''):
         self.git('checkout', '-B', branch, start_point, '--')
 
-    def push(self, branch, *, source_repo_url=None, force=False):
+    def push(self, branch, *, source_repo_url=None, force=False, skip_ci=False):
         self.git('checkout', branch, '--')
 
         self.git('diff-index', '--quiet', 'HEAD')  # check it is not dirty
@@ -146,7 +146,14 @@ def push(self, branch, *, source_repo_url=None, force=False):
         else:
             source = 'origin'
         force_flag = '--force' if force else ''
-        self.git('push', force_flag, source, '%s:%s' % (branch, branch))
+        # The following needs to be split into 2 variables as any whitespace in the string
+        # causes it to be quoted which makes the string ignored by git
+        if skip_ci:
+            skip_1 = '-o'
+            skip_2 = 'ci.skip'
+        else:
+            skip_1, skip_2 = '', ''
+        self.git('push', force_flag, skip_1, skip_2, source, '%s:%s' % (branch, branch))
 
     def get_commit_hash(self, rev='HEAD'):
         """Return commit hash for `rev` (default "HEAD")."""

marge/job.py

@@ -336,12 +336,14 @@ def push_force_to_mr(
         merge_request,
         branch_was_modified,
         source_repo_url=None,
+        skip_ci=False,
     ):
         try:
             self._repo.push(
                 merge_request.source_branch,
                 source_repo_url=source_repo_url,
                 force=True,
+                skip_ci=skip_ci
             )
         except git.GitError:
             def fetch_remote_branch():

pylintrc

@@ -30,6 +30,7 @@ max-line-length=110
 [DESIGN]
 max-args=10
 max-attributes=15
+max-locals=16
 max-public-methods=35
 
 [REPORTS]

tests/git_repo_mock.py

@@ -178,9 +178,11 @@ def merge(self, arg):
             self._local_repo.set_ref(self._branch, new_sha)
 
     def push(self, *args):
-        force_flag, remote_name, refspec = args
+        force_flag, skip_1, skip_2, remote_name, refspec = args
 
         assert force_flag in ('', '--force')
+        assert skip_1 in ('', '-o')
+        assert skip_2 in ('', 'ci.skip')
 
         branch, remote_branch = refspec.split(':')
         remote_url = self._remotes[remote_name]

tests/test_batch_job.py

@@ -7,10 +7,9 @@
 import marge.project
 import marge.user
 from marge.batch_job import BatchMergeJob, CannotBatch
-from marge.gitlab import GET
 from marge.job import CannotMerge, MergeJobOptions
 from marge.merge_request import MergeRequest
-from tests.gitlab_api_mock import MockLab, Ok, commit
+from tests.gitlab_api_mock import MockLab
 
 
 class TestBatchJob:
@@ -160,25 +159,3 @@ def test_fuse_mr_when_target_branch_was_moved(self, api, mocklab):
         with pytest.raises(CannotBatch) as exc_info:
             batch_merge_job.accept_mr(merge_request, 'abc')
         assert str(exc_info.value) == 'Someone was naughty and by-passed marge'
-
-    def test_fuse_mr_when_source_branch_was_moved(self, api, mocklab):
-        batch_merge_job = self.get_batch_merge_job(api, mocklab)
-        merge_request = self._mock_merge_request(
-            source_project_id=mocklab.merge_request_info['source_project_id'],
-            target_branch='master',
-            source_branch=mocklab.merge_request_info['source_branch'],
-        )
-
-        api.add_transition(
-            GET(
-                '/projects/{project_iid}/repository/branches/useless_new_feature'.format(
-                    project_iid=mocklab.merge_request_info['source_project_id'],
-                ),
-            ),
-            Ok({'commit': commit(commit_id='abc', status='running')}),
-        )
-
-        with pytest.raises(CannotMerge) as exc_info:
-            batch_merge_job.accept_mr(merge_request, mocklab.initial_master_sha)
-
-        assert str(exc_info.value) == 'Someone pushed to branch while we were trying to merge'