From f88bf087c3947990e28a57bb034028d4a3ff95fe Mon Sep 17 00:00:00 2001 From: Simon Westphahl Date: Fri, 18 Jan 2019 13:03:52 +0100 Subject: [PATCH] List changed files for all commits between refs Using git merge-bases will not work properly for some edge-cases. Using git rev-list will return the set of commits that are reachable from the PR head but without those already in the base branch. Change-Id: I25c75a70892bbac52db783fb63513f6aa37cb5f6 --- tests/base.py | 23 +++++++----- tests/unit/test_merger_repo.py | 66 ++++++++++++++++++++++++++++++++++ zuul/merger/merger.py | 17 ++++----- 3 files changed, 86 insertions(+), 20 deletions(-) diff --git a/tests/base.py b/tests/base.py index a6cc2f6781..8d52b755c5 100644 --- a/tests/base.py +++ b/tests/base.py @@ -2928,10 +2928,10 @@ class ZuulTestCase(BaseTestCase): zuul.merger.merger.reset_repo_to_head(repo) repo.git.clean('-x', '-f', '-d') - def create_branch(self, project, branch): + def create_branch(self, project, branch, commit_filename='README'): path = os.path.join(self.upstream_root, project) repo = git.Repo(path) - fn = os.path.join(path, 'README') + fn = os.path.join(path, commit_filename) branch_head = repo.create_head(branch) repo.head.reference = branch_head @@ -2952,15 +2952,20 @@ class ZuulTestCase(BaseTestCase): zuul.merger.merger.reset_repo_to_head(repo) repo.delete_head(repo.heads[branch], force=True) - def create_commit(self, project): + def create_commit(self, project, files=None, head='master', + message='Creating a fake commit', **kwargs): path = os.path.join(self.upstream_root, project) repo = git.Repo(path) - repo.head.reference = repo.heads['master'] - file_name = os.path.join(path, 'README') - with open(file_name, 'a') as f: - f.write('creating fake commit\n') - repo.index.add([file_name]) - commit = repo.index.commit('Creating a fake commit') + repo.head.reference = repo.heads[head] + repo.head.reset(index=True, working_tree=True) + + files = files or {"README": "creating fake commit\n"} + for name, content in files.items(): + file_name = os.path.join(path, name) + with open(file_name, 'a') as f: + f.write(content) + repo.index.add([file_name]) + commit = repo.index.commit(message, **kwargs) return commit.hexsha def orderedRelease(self, count=None): diff --git a/tests/unit/test_merger_repo.py b/tests/unit/test_merger_repo.py index 06c6314ecb..86323e8fbc 100644 --- a/tests/unit/test_merger_repo.py +++ b/tests/unit/test_merger_repo.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import datetime import logging import os @@ -204,6 +205,71 @@ class TestMergerRepo(ZuulTestCase): # And now reset the repo again. This should not crash work_repo.reset() + def test_files_changes(self): + parent_path = os.path.join(self.upstream_root, 'org/project1') + self.create_branch('org/project1', 'feature') + + work_repo = Repo(parent_path, self.workspace_root, + 'none@example.org', 'User Name', '0', '0') + changed_files = work_repo.getFilesChanges('feature', 'master') + + self.assertEqual(['README'], changed_files) + + def test_files_changes_master_fork_merges(self): + """Regression test for getFilesChanges() + + Check if correct list of changed files is listed for a messy + branch that has a merge of a fork, with the fork including a + merge of a new master revision. + + The previously used "git merge-base" approach did not handle this + case correctly. + """ + parent_path = os.path.join(self.upstream_root, 'org/project1') + repo = git.Repo(parent_path) + + self.create_branch('org/project1', 'messy', + commit_filename='messy1.txt') + + # Let time pass to reproduce the order for this error case + commit_date = datetime.datetime.now() + datetime.timedelta(seconds=5) + commit_date = commit_date.replace(microsecond=0).isoformat() + + # Create a commit on 'master' so we can merge it into the fork + files = {"master.txt": "master"} + master_ref = self.create_commit('org/project1', files=files, + message="Add master.txt", + commit_date=commit_date) + repo.refs.master.commit = master_ref + + # Create a fork of the 'messy' branch and merge + # 'master' into the fork (no fast-forward) + repo.create_head("messy-fork") + repo.heads["messy-fork"].commit = "messy" + repo.head.reference = 'messy' + repo.head.reset(index=True, working_tree=True) + repo.git.checkout('messy-fork') + repo.git.merge('master', no_ff=True) + + # Merge fork back into 'messy' branch (no fast-forward) + repo.head.reference = 'messy' + repo.head.reset(index=True, working_tree=True) + repo.git.checkout('messy') + repo.git.merge('messy-fork', no_ff=True) + + # Create another commit on top of 'messy' + files = {"messy2.txt": "messy2"} + messy_ref = self.create_commit('org/project1', files=files, + head='messy', message="Add messy2.txt") + repo.refs.messy.commit = messy_ref + + # Check that we get all changes for the 'messy' but not 'master' branch + work_repo = Repo(parent_path, self.workspace_root, + 'none@example.org', 'User Name', '0', '0') + changed_files = work_repo.getFilesChanges('messy', 'master') + self.assertEqual(sorted(['messy1.txt', 'messy2.txt']), + sorted(changed_files)) + class TestMergerWithAuthUrl(ZuulTestCase): config_file = 'zuul-github-driver.conf' diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py index c6eccaf195..52757c63d1 100644 --- a/zuul/merger/merger.py +++ b/zuul/merger/merger.py @@ -450,17 +450,12 @@ class Repo(object): head = repo.commit(self.revParse('FETCH_HEAD')) files = set() - merge_bases = [] - if tosha is not None: - merge_bases = repo.merge_base(head, tosha) - - files.update(set(head.stats.files.keys())) - if merge_bases: - hexsha_list = [b.hexsha for b in merge_bases] - for cmt in head.iter_parents(): - if cmt.hexsha in hexsha_list: - break - files.update(set(cmt.stats.files.keys())) + if tosha: + commit_diff = "{}..{}".format(tosha, head.hexsha) + for cmt in repo.iter_commits(commit_diff, no_merges=True): + files.update(cmt.stats.files.keys()) + else: + files.update(head.stats.files.keys()) return list(files) def deleteRemote(self, remote):