Optimistic batching for batch merging

README.md

@@ -381,16 +381,21 @@ extremely slow CI).
 ### How it works
 
 If marge-bot finds multiple merge requests to deal with, she attempts to create
-a batch job. She filters the merge requests such that they have all have a
+a batch job. This is initially a batch branch based on the latest master.
+She then filters the merge requests such that they have all have a
 common target branch, and eliminates those that have not yet passed CI (a
 heuristic to help guarantee the batch will pass CI later).
 
-Once the merge requests have been gathered, a batch branch is created using the
-commits from each merge request in sequence. Any merge request that cannot be
+Once the merge requests have been gathered, trailers are added to their branches one 
+by one, these changes are pushed to their remotes, they are rebased on the batch branch
+and the rebased version is also pushed to the remote of the individual MR. This
+is to ensure that the commit SHAs of the changes in the individual MRs match
+those in the batch branch. This is very important later. Any merge request that cannot be
 merged to this branch (e.g. due to a rebase conflict) is filtered out. A new
 merge request is then created for this branch, and tested in CI.
 
-If CI passes, the original merge requests will be merged one by one.
+If CI passes, the batch branch is merged which merges all individual constituent
+MRs (this is why the SHAs must match).
 
 If the batch job fails for any reason, we fall back to merging the first merge
 request, before attempting a new batch job.
@@ -406,16 +411,10 @@ request, before attempting a new batch job.
   each source branch, because we know the final linearization of all commits
   passes in that all MRs passed individually on their branches.
 
-* As trailers are added to the original merge requests only, their branches
-  would need to be pushed to in order to reflect this change. This would trigger
-  CI in each of the branches again that would have to be passed before merging,
-  which effectively defeats the point of batching. To workaround this, the
-  current implementation merges to the target branch through git, instead of the
-  GitLab API. GitLab will detect the merge request as having been merged, and
-  update the merge request status accordingly, regardless of whether it has
-  passed CI. This does still mean the triggered CI jobs will be running even
-  though the merge requests are merged. marge-bot will attempt to cancel these
-  pipelines, although this doesn't work too effectively if external CI is used.
+* Because trailers are added during this process and pushed, this would trigger
+  CI pipelines for each branch to be merged. We use the `ci.skip` option to prevent
+  unnecessary CI runs which leaves skipped pipelines appearing on the Pipelines
+  page for the repo. However this is just a visual issue.
 
 * There is what can be considered to be a flaw in this implementation that could
   potentially result in a non-green master; consider the following situation:

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'