Consider commit metadata changes as a change

Summary: This diff makes kpd treat the number of commits and the contents of said commits as significant. Before, kpd would treat the diff contents as significant, leading to series that only split changes out into separate commits to being un-updated. See https://github.com/facebookincubator/kernel-patches-daemon/blob/543eb6e78e79a112e8ba89bb6440b627fbe16468/kernel_patches_daemon/branch_worker.py#L972-L973 .

Reviewed By: chantra, danielocfb

Differential Revision: D63914823

fbshipit-source-id: 0f710a0076d454d94198f5495795d26695629129

Author: Daniel Xu

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

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", "[email protected]")
+
+        # 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