From e63d7b0cdb4a38fd2abbd962e77800dac8c2bd22 Mon Sep 17 00:00:00 2001 From: Dong Zhang Date: Wed, 10 Nov 2021 12:50:55 +0800 Subject: [PATCH] Fix a bug in getting changed files The original implementation takes into account the changed fils from all commits of a PR. It causes a bug when files get changed and reverted in those commits. e.g. A file is added in first commit then removed in second commit, this file should should not be considered as a changed file in the PR. Change-Id: I7db8b9d3f3267073c5e1a71f52e75939ffa91773 --- tests/base.py | 11 +++++++++-- tests/unit/test_merger_repo.py | 27 +++++++++++++++++++++++++++ zuul/merger/merger.py | 6 +++--- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/tests/base.py b/tests/base.py index ad6e53226a..92a5f6ddd1 100644 --- a/tests/base.py +++ b/tests/base.py @@ -5175,8 +5175,9 @@ class ZuulTestCase(BaseTestCase): repo.head.reset(working_tree=True) repo.delete_head(repo.heads[branch], force=True) - def create_commit(self, project, files=None, head='master', - message='Creating a fake commit', **kwargs): + def create_commit(self, project, files=None, delete_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[head] @@ -5188,6 +5189,12 @@ class ZuulTestCase(BaseTestCase): with open(file_name, 'a') as f: f.write(content) repo.index.add([file_name]) + + delete_files = delete_files or [] + for name in delete_files: + file_name = os.path.join(path, name) + repo.index.remove([file_name]) + commit = repo.index.commit(message, **kwargs) return commit.hexsha diff --git a/tests/unit/test_merger_repo.py b/tests/unit/test_merger_repo.py index 7e49a4fb62..4ce595b64a 100644 --- a/tests/unit/test_merger_repo.py +++ b/tests/unit/test_merger_repo.py @@ -353,6 +353,33 @@ class TestMergerRepo(ZuulTestCase): self.assertEqual(['README'], changed_files) + def test_files_changes_add_and_remove_files(self): + """ + If the changed files in previous commits are reverted in later commits, + they should not be considered as changed in the PR. + """ + parent_path = os.path.join(self.upstream_root, 'org/project1') + self.create_branch('org/project1', 'feature1') + + work_repo = Repo(parent_path, self.workspace_root, + 'none@example.org', 'User Name', '0', '0') + + # Add a file in first commit + files = {'to-be-deleted.txt': 'test'} + self.create_commit('org/project1', files=files, head='feature1', + message='Add file') + changed_files = work_repo.getFilesChanges('feature1', 'master') + self.assertEqual(sorted(['README', 'to-be-deleted.txt']), + sorted(changed_files)) + + # Delete the file in second commit + delete_files = ['to-be-deleted.txt'] + self.create_commit('org/project1', files={}, + delete_files=delete_files, head='feature1', + message='Delete file') + changed_files = work_repo.getFilesChanges('feature1', 'master') + self.assertEqual(['README'], changed_files) + def test_files_changes_master_fork_merges(self): """Regression test for getFilesChanges() diff --git a/zuul/merger/merger.py b/zuul/merger/merger.py index 8f5a61aa2f..b736d3a647 100644 --- a/zuul/merger/merger.py +++ b/zuul/merger/merger.py @@ -673,9 +673,9 @@ class Repo(object): files = set() 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()) + head_commit = repo.commit(head.hexsha) + diff_index = head_commit.diff(tosha) + files.update((item.a_path for item in diff_index)) else: files.update(head.stats.files.keys()) return list(files)