diff --git a/kernel_patches_daemon/branch_worker.py b/kernel_patches_daemon/branch_worker.py index 8a01ca7..3abcae8 100644 --- a/kernel_patches_daemon/branch_worker.py +++ b/kernel_patches_daemon/branch_worker.py @@ -448,12 +448,29 @@ async def _series_already_applied(repo: git.Repo, series: Series) -> bool: return any(ps.lower() in summaries for ps in await series.patch_subjects()) -def _is_branch_changed(repo: git.Repo, old_branch: str, new_branch: str) -> bool: +def _is_branch_changed( + repo: git.Repo, base_branch: str, old_branch: str, new_branch: str +) -> bool: """ Returns whether or not the new_branch (in local repo) is different from the old_branch (remote repo). + + Note that care is taken to avoid detecting commit SHA differences. The + SHA is unstable even if the contents are the same. So compare the contents + we care about. """ - return repo.git.diff(new_branch, old_branch) + # Check if code changes are different + if repo.git.diff(new_branch, old_branch): + return True + + # Check if change metadata is different (number of commits and + # commit messages) + old_metadata = repo.git.log('--format="%B"', f"{base_branch}..{old_branch}") + new_metadata = repo.git.log('--format="%B"', f"{base_branch}..{new_branch}") + if old_metadata != new_metadata: + return True + + return False def _is_outdated_pr(pr: PullRequest) -> bool: @@ -932,12 +949,15 @@ async def apply_push_comment( has_merge_conflict=True, can_create=True, ) - # force push only if if's a new branch or there is code diffs between old and new branches + # force push only if if's a new branch or there is code or metadata diffs between old and new branches # which could mean that we applied new set of patches or just rebased if branch_name in self.branches and ( branch_name not in self.all_prs # NO PR yet or _is_branch_changed( - self.repo_local, f"remotes/origin/{branch_name}", branch_name + self.repo_local, + f"remotes/origin/{self.repo_pr_base_branch}", + f"remotes/origin/{branch_name}", + branch_name, ) # have code changes ): # we have branch, but either NO PR or there is code changes, we must try to diff --git a/tests/test_branch_worker.py b/tests/test_branch_worker.py index 5489443..4465096 100644 --- a/tests/test_branch_worker.py +++ b/tests/test_branch_worker.py @@ -6,6 +6,7 @@ # pyre-unsafe +import os import random import re import shutil @@ -23,6 +24,7 @@ from freezegun import freeze_time from git.exc import GitCommandError from kernel_patches_daemon.branch_worker import ( + _is_branch_changed, _series_already_applied, ALREADY_MERGED_LOOKBACK, BRANCH_TTL, @@ -983,6 +985,153 @@ async def test_applied_all_case_insensitive(self, m: aioresponses): self.assertTrue(await _series_already_applied(self.repo, series)) +class TestBranchChanged(unittest.TestCase): + SINGLE_COMMIT_CHANGE_MESSAGE = "single commit change\n" + + def setUp(self): + self.tmp_dir = tempfile.mkdtemp() + self.repo = git.Repo.init(self.tmp_dir) + + # So git-commit --amend doesn't error about missing configs in CI + self.repo.git.config("user.name", "test") + self.repo.git.config("user.email", "test@test.com") + + # Create a file in the repository + with open(self.tmp_dir + "/file.txt", "w") as f: + f.write("Hello, world!\n") + + # Create initial commit on master + self.repo.index.add(["file.txt"]) + self.repo.index.commit("Initial commit\n") + + # Create new branch for single commit change + self.repo.create_head("single_commit_change", commit="master").checkout() + with open(f"{self.tmp_dir}/file.txt", "a") as f: + f.write("line 1\n") + f.write("line 2\n") + self.repo.index.add(["file.txt"]) + self.repo.index.commit(self.SINGLE_COMMIT_CHANGE_MESSAGE) + + # Create branch for a different single commit change + self.repo.create_head( + "different_single_commit_change", commit="master" + ).checkout() + with open(f"{self.tmp_dir}/file.txt", "a") as f: + f.write("built different\n") + self.repo.index.add(["file.txt"]) + self.repo.index.commit("different single commit change\n") + + # Create duplicate branch for single commit change but with different SHA + self.repo.create_head( + "single_commit_change_clone", commit="single_commit_change" + ).checkout() + self.repo.git.commit("--amend", "--no-edit") + + # Create branch to do same change but in two commits + self.repo.create_head("two_commit_change", commit="master").checkout() + for i in range(2): + with open(f"{self.tmp_dir}/file.txt", "a") as f: + f.write(f"line {i+1}\n") + self.repo.index.add(["file.txt"]) + self.repo.index.commit("split change, part {i+1}\n") + + # Create branch to do same change but in two commits with same duplicate + self.repo.create_head( + "two_commit_change_with_same_msg", commit="master" + ).checkout() + for i in range(2): + with open(f"{self.tmp_dir}/file.txt", "a") as f: + f.write(f"line {i+1}\n") + self.repo.index.add(["file.txt"]) + self.repo.index.commit(self.SINGLE_COMMIT_CHANGE_MESSAGE) + + self.repo.heads.master.checkout() + + def tearDown(self): + shutil.rmtree(self.tmp_dir) + + def test_different_change(self): + self.assertTrue( + _is_branch_changed( + self.repo, + "master", + "single_commit_change", + "different_single_commit_change", + ) + ) + + def test_duplicate_change(self): + self.assertFalse( + _is_branch_changed( + self.repo, + "master", + "single_commit_change", + "single_commit_change", + ) + ) + self.assertFalse( + _is_branch_changed( + self.repo, + "master", + "two_commit_change", + "two_commit_change", + ) + ) + self.assertFalse( + _is_branch_changed( + self.repo, + "master", + "two_commit_change_with_same_msg", + "two_commit_change_with_same_msg", + ) + ) + self.assertFalse( + _is_branch_changed( + self.repo, + "master", + "single_commit_change_clone", + "single_commit_change_clone", + ) + ) + self.assertFalse( + _is_branch_changed( + self.repo, + "master", + "single_commit_change", + "single_commit_change_clone", + ) + ) + + def test_split_change(self): + # Double check that there are no source changes + self.repo.heads.single_commit_change.checkout() + self.assertFalse(self.repo.git.diff("two_commit_change")) + + # But the varying number of commits triggers a change detection + self.assertTrue( + _is_branch_changed( + self.repo, + "master", + "single_commit_change", + "two_commit_change", + ) + ) + + def test_split_duplicate_message_change(self): + # Double check that there are no source changes + self.repo.heads.single_commit_change.checkout() + self.assertFalse(self.repo.git.diff("two_commit_change_with_same_msg")) + + self.assertTrue( + _is_branch_changed( + self.repo, + "master", + "single_commit_change", + "two_commit_change_with_same_msg", + ) + ) + + class TestEmailNotificationBody(unittest.TestCase): # Always show full diff on string match failures maxDiff = None