Merge "merger: Keep redundant cherry-pick commits"

4 files changed, 121 insertions, 8 deletions

diff --git a/releasenotes/notes/handle-existing-commits-with-cherry-pick-e1a979c2e7ed1a78.yaml b/releasenotes/notes/handle-existing-commits-with-cherry-pick-e1a979c2e7ed1a78.yaml
new file mode 100644
index 000000000..dd5c502d2
--- /dev/null
+++ b/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.
diff --git a/tests/base.py b/tests/base.py
index fd927a92c..290d51934 100644
--- a/tests/base.py
+++ b/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
 
diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py
index 172ed34dc..131034f17 100644
--- a/tests/unit/test_scheduler.py
+++ b/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')
diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py
index e4688a1b7..1df833bc5 100644
--- a/zuul/merger/merger.py
+++ b/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):