merger: Keep redundant cherry-pick commits

In normal git usage, cherry-picking a commit that has already been
applied and doesn't do anything or cherry-picking an empty commit causes
git to exit with an error to let the user decide what they want to do.
However, this doesn't match the behavior of merges and rebases where
non-empty commits that have already been applied are simply skipped
(empty source commits are preserved).

To fix this, add the --keep-redundant-commit option to `git cherry-pick`
to make git always keep a commit when cherry-picking even when it is
empty for either reason. Then, after the cherry-pick, check if the new
commit is empty and if so back it out if the original commit _wasn't_
empty.

This two step process is necessary because git doesn't have any options
to simply skip cherry-pick commits that have already been applied to the
tree.

Removing commits that have already been applied is particularly
important in a "deploy" pipeline triggered by a Gerrit "change-merged"
event, since the scheduler will try to cherry-pick the change on top of
the commit that just merged. Without this option, the cherry-pick will
fail and the deploy pipeline will fail with a MERGE_CONFICT.

Change-Id: I326ba49e2268197662d11fd79e46f3c020675f21

releasenotes/notes/handle-existing-commits-with-cherry-pick-e1a979c2e7ed1a78.yaml

@@ -0,0 +1,14 @@
+---
+fixes:
+  - |
+    The `cherry-pick` merge mode will now silently skip commits that have
+    already been applied to the tree when cherry-picking, instead of failing
+    with an error.
+
+    The exception to this is if the source of the cherry-pick is an empty
+    commit, in which case it is always kept.
+
+    Skipping commits that have already been applied is important in a pipeline
+    triggered by the Gerrit `change-merged` event (like the `deploy` pipeline),
+    since the scheduler would previously try to cherry-pick the change on top
+    of the commit that just merged and fail.

tests/base.py

@@ -384,7 +384,7 @@ class FakeGerritChange(object):
     def __init__(self, gerrit, number, project, branch, subject,
                  status='NEW', upstream_root=None, files={},
                  parent=None, merge_parents=None, merge_files=None,
-                 topic=None):
+                 topic=None, empty=False):
         self.gerrit = gerrit
         self.source = gerrit
         self.reported = 0
@@ -429,7 +429,7 @@ class FakeGerritChange(object):
             self.addMergePatchset(parents=merge_parents,
                                   merge_files=merge_files)
         else:
-            self.addPatchset(files=files, parent=parent)
+            self.addPatchset(files=files, parent=parent, empty=empty)
         if merge_parents:
             self.data['parents'] = merge_parents
         elif parent:
@@ -503,9 +503,11 @@ class FakeGerritChange(object):
         repo.heads['master'].checkout()
         return r
 
-    def addPatchset(self, files=None, large=False, parent=None):
+    def addPatchset(self, files=None, large=False, parent=None, empty=False):
         self.latest_patchset += 1
-        if not files:
+        if empty:
+            files = {}
+        elif not files:
             fn = '%s-%s' % (self.branch.replace('/', '_'), self.number)
             data = ("test %s %s %s\n" %
                     (self.branch, self.number, self.latest_patchset))
@@ -1330,7 +1332,7 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
 
     def addFakeChange(self, project, branch, subject, status='NEW',
                       files=None, parent=None, merge_parents=None,
-                      merge_files=None, topic=None):
+                      merge_files=None, topic=None, empty=False):
         """Add a change to the fake Gerrit."""
         self.change_number += 1
         c = FakeGerritChange(self, self.change_number, project, branch,
@@ -1338,7 +1340,7 @@ class FakeGerritConnection(gerritconnection.GerritConnection):
                              status=status, files=files, parent=parent,
                              merge_parents=merge_parents,
                              merge_files=merge_files,
-                             topic=topic)
+                             topic=topic, empty=empty)
         self.changes[self.change_number] = c
         return c
 

tests/unit/test_scheduler.py

@@ -7440,6 +7440,85 @@ class TestSchedulerMerges(ZuulTestCase):
         result = self._test_project_merge_mode('cherry-pick')
         self.assertEqual(result, expected_messages)
 
+    def test_project_merge_mode_cherrypick_redundant(self):
+        # A redundant commit (that is, one that has already been applied to the
+        # working tree) should be skipped
+        self.executor_server.keep_jobdir = False
+        project = 'org/project-cherry-pick'
+        files = {
+            "foo.txt": "ABC",
+        }
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A', files=files)
+        A.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        self.executor_server.hold_jobs_in_build = True
+        B = self.fake_gerrit.addFakeChange(project, 'master', 'B', files=files)
+        B.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(B.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        build = self.builds[-1]
+        path = os.path.join(build.jobdir.src_root, 'review.example.com',
+                            project)
+        repo = git.Repo(path)
+        repo_messages = [c.message.strip() for c in repo.iter_commits()]
+        repo_messages.reverse()
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        expected_messages = [
+            'initial commit',
+            'add content from fixture',
+            'A-1',
+        ]
+        self.assertHistory([
+            dict(name='project-test1', result='SUCCESS', changes='1,1'),
+            dict(name='project-test1', result='SUCCESS', changes='2,1'),
+        ])
+        self.assertEqual(A.data['status'], 'MERGED')
+        self.assertEqual(B.data['status'], 'MERGED')
+        self.assertEqual(repo_messages, expected_messages)
+
+    def test_project_merge_mode_cherrypick_empty(self):
+        # An empty commit (that is, one that doesn't modify any files) should
+        # be preserved
+        self.executor_server.keep_jobdir = False
+        project = 'org/project-cherry-pick'
+        self.executor_server.hold_jobs_in_build = True
+        A = self.fake_gerrit.addFakeChange(project, 'master', 'A', empty=True)
+        A.addApproval('Code-Review', 2)
+        self.fake_gerrit.addEvent(A.addApproval('Approved', 1))
+        self.waitUntilSettled()
+
+        build = self.builds[-1]
+        path = os.path.join(build.jobdir.src_root, 'review.example.com',
+                            project)
+        repo = git.Repo(path)
+        repo_messages = [c.message.strip() for c in repo.iter_commits()]
+        repo_messages.reverse()
+
+        changed_files = list(repo.commit("HEAD").diff(repo.commit("HEAD~1")))
+        self.assertEqual(changed_files, [])
+
+        self.executor_server.hold_jobs_in_build = False
+        self.executor_server.release()
+        self.waitUntilSettled()
+
+        expected_messages = [
+            'initial commit',
+            'add content from fixture',
+            'A-1',
+        ]
+        self.assertHistory([
+            dict(name='project-test1', result='SUCCESS', changes='1,1'),
+        ])
+        self.assertEqual(A.data['status'], 'MERGED')
+        self.assertEqual(repo_messages, expected_messages)
+
     def test_project_merge_mode_cherrypick_branch_merge(self):
         "Test that branches can be merged together in cherry-pick mode"
         self.create_branch('org/project-merge-branches', 'mp')

zuul/merger/merger.py

@@ -595,14 +595,32 @@ class Repo(object):
         log = get_annotated_logger(self.log, zuul_event_id)
         repo = self.createRepoObject(zuul_event_id)
         self.fetch(ref, zuul_event_id=zuul_event_id)
-        if len(repo.commit("FETCH_HEAD").parents) > 1:
+        fetch_head = repo.commit("FETCH_HEAD")
+        if len(fetch_head.parents) > 1:
             args = ["-s", "resolve", "FETCH_HEAD"]
             log.debug("Merging %s with args %s instead of cherry-picking",
                       ref, args)
             repo.git.merge(*args)
         else:
             log.debug("Cherry-picking %s", ref)
-            repo.git.cherry_pick("FETCH_HEAD")
+            # Git doesn't have an option to ignore commits that are already
+            # applied to the working tree when cherry-picking, so pass the
+            # --keep-redundant-commits option, which will cause it to make an
+            # empty commit
+            repo.git.cherry_pick("FETCH_HEAD", keep_redundant_commits=True)
+
+            # If the newly applied commit is empty, it means either:
+            #  1) The commit being cherry-picked was empty, in which the empty
+            #     commit should be kept
+            #  2) The commit being cherry-picked was already applied to the
+            #     tree, in which case the empty commit should be backed out
+            head = repo.commit("HEAD")
+            parent = head.parents[0]
+            if not any(head.diff(parent)) and \
+                    any(fetch_head.diff(fetch_head.parents[0])):
+                log.debug("%s was already applied. Removing it", ref)
+                self._checkout(repo, parent)
+
         return repo.head.commit
 
     def merge(self, ref, strategy=None, zuul_event_id=None):