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 0000000000..dd5c502d2b --- /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 fd927a92cf..290d519346 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 172ed34dca..131034f178 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 e4688a1b7c..1df833bc56 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):