Improve batching: allow batch MRs use merge commits

.gitignore

@@ -8,3 +8,6 @@ ENV/
 # nix stuff
 result
 result-*
+
+# Editor
+*.sw[po]

marge/app.py

@@ -213,6 +213,17 @@ def regexp(str_regex):
         action='store_true',
         help='Disable fast forwarding when merging MR batches'
     )
+    parser.add_argument(
+        '--use-merge-commit-batches',
+        action='store_true',
+        help='Use merge commit when creating batches, so that the commits in the batch MR '
+             'will be the same with in individual MRs. Requires sudo scope in the access token.\n',
+    )
+    parser.add_argument(
+        '--skip-ci-batches',
+        action='store_true',
+        help='Skip CI when updating individual MRs when using batches'
+    )
     config = parser.parse_args(args)
 
     if config.use_merge_strategy and config.batch:
@@ -312,6 +323,8 @@ def main(args=None):
                 ci_timeout=options.ci_timeout,
                 fusion=fusion,
                 use_no_ff_batches=options.use_no_ff_batches,
+                use_merge_commit_batches=options.use_merge_commit_batches,
+                skip_ci_batches=options.skip_ci_batches,
             ),
             batch=options.batch,
         )

marge/approvals.py

@@ -51,11 +51,15 @@ def reapprove(self):
         (which may invalidate approvals, depending on GitLab settings) and then
         restore the approval status.
         """
+        self.approve(self)
+
+    def approve(self, obj):
+        """Approve an object which can be a merge_request or an approval."""
         if self._api.version().release >= (9, 2, 2):
-            approve_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approve'.format(self)
+            approve_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approve'.format(obj)
         else:
             # GitLab botched the v4 api before 9.2.3
-            approve_url = '/projects/{0.project_id}/merge_requests/{0.id}/approve'.format(self)
+            approve_url = '/projects/{0.project_id}/merge_requests/{0.id}/approve'.format(obj)
 
         for uid in self.approver_ids:
             self._api.call(POST(approve_url), sudo=uid)

marge/batch_job.py

@@ -1,8 +1,9 @@
-# pylint: disable=too-many-branches,too-many-statements
+# pylint: disable=too-many-branches,too-many-statements,arguments-differ
 import logging as log
 from time import sleep
 
 from . import git
+from . import gitlab
 from .commit import Commit
 from .job import MergeJob, CannotMerge, SkipMerge
 from .merge_request import MergeRequest
@@ -46,6 +47,7 @@ def close_batch_mr(self):
             batch_mr.close()
 
     def create_batch_mr(self, target_branch):
+        self.push_batch()
         log.info('Creating batch MR')
         params = {
             'source_branch': BatchMergeJob.BATCH_BRANCH_NAME,
@@ -68,10 +70,10 @@ def get_mrs_with_common_target_branch(self, target_branch):
             if merge_request.target_branch == target_branch
         ]
 
-    def ensure_mergeable_mr(self, merge_request):
+    def ensure_mergeable_mr(self, merge_request, skip_ci=False):
         super().ensure_mergeable_mr(merge_request)
 
-        if self._project.only_allow_merge_if_pipeline_succeeds:
+        if self._project.only_allow_merge_if_pipeline_succeeds and not skip_ci:
             ci_status = self.get_mr_ci_status(merge_request)
             if ci_status != 'success':
                 raise CannotBatch('This MR has not passed CI.')
@@ -121,46 +123,61 @@ def merge_batch(self, target_branch, source_branch, no_ff=False):
             source_branch,
         )
 
-    def accept_mr(
+    def update_merge_request(
         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()
 
-        # Make sure latest commit in remote <target_branch> is the one we tested against
-        new_target_sha = Commit.last_on_branch(self._project.id, merge_request.target_branch, self._api).id
-        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,
+            skip_ci=self._options.skip_ci_batches,
         )
 
         sha_now = Commit.last_on_branch(
             merge_request.source_project_id, merge_request.source_branch, self._api,
         ).id
+        log.info('update_merge_request: sha_now (%s), actual_sha (%s)', sha_now, actual_sha)
         # 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.
+        # As we're not using the API to merge the individual 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)
+        return sha_now
+
+    def accept_mr(
+        self,
+        merge_request,
+        expected_remote_target_branch_sha,
+        source_repo_url=None,
+    ):
+        log.info('Accept MR !%s', merge_request.iid)
+
+        # Make sure latest commit in remote <target_branch> is the one we tested against
+        new_target_sha = Commit.last_on_branch(self._project.id, merge_request.target_branch, self._api).id
+        if new_target_sha != expected_remote_target_branch_sha:
+            raise CannotBatch('Someone was naughty and by-passed marge')
+
+        # Rebase and apply the trailers
+        self.update_merge_request(
+            merge_request,
+            source_repo_url=source_repo_url,
+        )
 
         # This switches git to <target_branch>
         final_sha = self.merge_batch(
             merge_request.target_branch,
             merge_request.source_branch,
             self._options.use_no_ff_batches,
         )
-
         # Don't force push in case the remote has changed.
         self._repo.push(merge_request.target_branch, force=False)
 
@@ -203,6 +220,11 @@ def execute(self):
         self._repo.checkout_branch(target_branch, 'origin/%s' % target_branch)
         self._repo.checkout_branch(BatchMergeJob.BATCH_BRANCH_NAME, 'origin/%s' % target_branch)
 
+        batch_mr = self.create_batch_mr(
+            target_branch=target_branch,
+        )
+        batch_mr_sha = batch_mr.sha
+
         working_merge_requests = []
 
         for merge_request in merge_requests:
@@ -213,42 +235,65 @@ def execute(self):
                     '%s/%s' % (merge_request_remote, merge_request.source_branch),
                 )
 
-                # Update <source_branch> on latest <batch> branch so it contains previous MRs
-                self.fuse(
-                    merge_request.source_branch,
-                    BatchMergeJob.BATCH_BRANCH_NAME,
-                    source_repo_url=source_repo_url,
-                    local=True,
-                )
-
-                # Update <batch> branch with MR changes
-                self._repo.fast_forward(
-                    BatchMergeJob.BATCH_BRANCH_NAME,
-                    merge_request.source_branch,
-                    local=True,
-                )
+                if self._options.use_merge_commit_batches:
+                    # Rebase and apply the trailers before running the batch MR
+                    actual_sha = self.update_merge_request(
+                        merge_request,
+                        source_repo_url=source_repo_url,
+                    )
+                    # Update <batch> branch with MR changes
+                    batch_mr_sha = self._repo.merge(
+                        BatchMergeJob.BATCH_BRANCH_NAME,
+                        merge_request.source_branch,
+                        '-m',
+                        'Batch merge !%s into %s (!%s)' % (
+                            merge_request.iid,
+                            merge_request.target_branch,
+                            batch_mr.iid
+                        ),
+                        local=True,
+                    )
+                else:
+                    # Update <source_branch> on latest <batch> branch so it contains previous MRs
+                    self.fuse(
+                        merge_request.source_branch,
+                        BatchMergeJob.BATCH_BRANCH_NAME,
+                        source_repo_url=source_repo_url,
+                        local=True,
+                    )
+                    # Update <batch> branch with MR changes
+                    self._repo.fast_forward(
+                        BatchMergeJob.BATCH_BRANCH_NAME,
+                        merge_request.source_branch,
+                        local=True,
+                    )
 
                 # We don't need <source_branch> anymore. Remove it now in case another
                 # merge request is using the same branch name in a different project.
-                # FIXME: is this actually needed?
                 self._repo.remove_branch(merge_request.source_branch)
             except git.GitError:
                 log.warning('Skipping MR !%s, got conflicts while rebasing', merge_request.iid)
                 continue
             else:
+                if self._options.use_merge_commit_batches:
+                    # update merge_request with the current sha, we will compare it with
+                    # the actual sha later to make sure no one pushed this MR meanwhile
+                    merge_request.update_sha(actual_sha)
+
                 working_merge_requests.append(merge_request)
+
         if len(working_merge_requests) <= 1:
             raise CannotBatch('not enough ready merge requests')
+
+        # This switches git to <batch> branch
+        self.push_batch()
+        for merge_request in working_merge_requests:
+            merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid))
+
+        # wait for the CI of the batch MR
         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,
-            )
-            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)
+                self.wait_for_ci_to_pass(batch_mr, commit_sha=batch_mr_sha)
             except CannotMerge as err:
                 for merge_request in working_merge_requests:
                     merge_request.comment(
@@ -258,17 +303,23 @@ def execute(self):
                         ),
                     )
                 raise CannotBatch(err.reason) from err
+
+        # check each sub MR, and accept each sub MR if using the normal batch
         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,
-                )
+                # we know the batch MR's CI passed, so we skip CI for sub MRs this time
+                self.ensure_mergeable_mr(merge_request, skip_ci=True)
+
+                if not self._options.use_merge_commit_batches:
+                    # accept each MRs
+                    remote_target_branch_sha = self.accept_mr(
+                        merge_request,
+                        remote_target_branch_sha,
+                        source_repo_url=source_repo_url,
+                    )
             except CannotBatch as err:
                 merge_request.comment(
                     "I couldn't merge this branch: {error} I will retry later...".format(
@@ -283,3 +334,24 @@ def execute(self):
                 self.unassign_from_mr(merge_request)
                 merge_request.comment("I couldn't merge this branch: %s" % err.reason)
                 raise
+
+        # Accept the batch MR
+        if self._options.use_merge_commit_batches:
+            # Approve the batch MR using the last sub MR's approvers
+            if not batch_mr.fetch_approvals().sufficient:
+                approvals = working_merge_requests[-1].fetch_approvals()
+                try:
+                    approvals.approve(batch_mr)
+                except (gitlab.Forbidden, gitlab.Unauthorized):
+                    log.exception('Failed to approve MR:')
+
+            try:
+                ret = batch_mr.accept(
+                    remove_branch=batch_mr.force_remove_source_branch,
+                    sha=batch_mr_sha,
+                    merge_when_pipeline_succeeds=bool(self._project.only_allow_merge_if_pipeline_succeeds),
+                )
+                log.info('batch_mr.accept result: %s', ret)
+            except gitlab.ApiError as err:
+                log.exception('Gitlab API Error:')
+                raise CannotMerge('Gitlab API Error: %s' % err)

marge/git.py

@@ -129,9 +129,10 @@ def remove_branch(self, branch, *, new_current_branch='master'):
         self.git('branch', '-D', branch)
 
     def checkout_branch(self, branch, start_point=''):
-        self.git('checkout', '-B', branch, start_point, '--')
+        create_and_reset = '-B' if start_point else ''
+        self.git('checkout', create_and_reset, 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 +147,8 @@ 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))
+        skip_flag = ('-o', 'ci.skip') if skip_ci else ()
+        self.git('push', force_flag, *skip_flag, source, '%s:%s' % (branch, branch))
 
     def get_commit_hash(self, rev='HEAD'):
         """Return commit hash for `rev` (default "HEAD")."""