check_kernel_commits.py: Fix corner case around Fixes: commits (cve-bf)

check_kernel_commits.py

@@ -7,6 +7,7 @@
 import textwrap
 
 from kt.ktlib.ciq_helpers import (
+    CIQ_filter_unapplied_commits,
     CIQ_find_fixes_in_mainline,
     CIQ_find_matching_cve,
     CIQ_get_commit_body,
@@ -126,6 +127,9 @@ def main():
     any_findings = False
     out_lines = []
 
+    # len(pr_commits) >= 1
+    oldest_pr_commit = pr_commits[-1]
+    newest_pr_commit = pr_commits[0]
     for sha in reversed(pr_commits):  # oldest first
         short_hash, subject = get_short_hash_and_subject(args.repo, sha)
         pr_commit_desc = f"{short_hash} ({subject})"
@@ -155,7 +159,68 @@ def main():
                     )
                     out_lines.append("")  # blank line
                 continue
-            fixes = CIQ_find_fixes_in_mainline(args.repo, args.pr_branch, upstream_ref, uhash)
+
+            # Find commits that contains Fixes: <uhash> in upstream_ref
+            fixes = CIQ_find_fixes_in_mainline(repo=args.repo, upstream_ref=upstream_ref, hash_=uhash)
+
+            # Filter the above commits if they were unapplied in the pr_branch (whole fetched history is used)
+            fixes_unapplied_without_limit = CIQ_filter_unapplied_commits(
+                repo=args.repo, pr_branch=args.pr_branch, commits=fixes
+            )
+
+            # Filter the above commits if they were unapplied in the pr_branch (oldest_pr_commit..newest_pr_commit interval is used)
+            fixes_unapplied_with_limit = CIQ_filter_unapplied_commits(
+                repo=args.repo, pr_branch=args.pr_branch, commits=fixes, interval=(oldest_pr_commit, newest_pr_commit)
+            )
+
+            # Issue a warning if we found that a commit that contains Fixes: <uhash> has been applied before <uhash>
+            # Very specific case, but check this PR where it did happen https://github.com/ctrliq/kernel-src-tree/pull/1212#issuecomment-4421837799
+            # It is up to the developer to assess the situation if this happens. In the example case, a commit X was pushed, then it was reverted by commit Y,
+            # then commit X was pushed again but automation locally did not flag that it was reverted before because its dependency Y appeared to have been applied
+            # in the local tree.
+            fixes_applied_before_commit = set(fixes_unapplied_with_limit) - set(fixes_unapplied_without_limit)
+            if fixes_applied_before_commit:
+                # Build the fixes display text
+                fixes_lines = []
+                for fix_hash, display_str in fixes_applied_before_commit:
+                    fixes_lines.append(display_str)
+
+                fixes_text = "\n".join(fixes_lines)
+                any_findings = True
+                if args.markdown:
+                    fixes_block = "    " + fixes_text.replace("\n", "\n    ")
+                    out_lines.append(
+                        f"- ❗ PR commit `{pr_commit_desc}` references upstream commit  \n"
+                        f"  `{short_uhash}` which has been referenced by a `Fixes:` tag in the upstream  \n"
+                        f"  Linux kernel:\n\n"
+                        f"```text\n{fixes_block}\n```\n"
+                        "   was applied after its deps, not before\n"
+                    )
+                else:
+                    prefix = "[FIXES-WRONG-ORDER] "
+                    header = (
+                        f"{prefix}PR commit {pr_commit_desc} references upstream commit "
+                        f"{short_uhash}, which has Fixes tags:"
+                    )
+                    out_lines.append(
+                        wrap_paragraph(
+                            header, width=80, initial_indent="", subsequent_indent=" " * len(prefix)
+                        )  # spaces for '[FIXES-WRONG-ORDER] '
+                    )
+                    out_lines.append("")  # blank line after 'Fixes tags:'
+                    for line in fixes_text.splitlines():
+                        out_lines.append("    " + line)
+
+                    out_lines.append(
+                        wrap_paragraph(
+                            text="was applied after its deps, not before\n",
+                            initial_indent=" " * len(prefix),
+                        )
+                    )
+                    out_lines.append("")  # blank line
+
+            # We continue the usual checks for fixes that were unapplied after the commit of interest
+            fixes = fixes_unapplied_with_limit
             if fixes:
                 any_findings = True
 

ciq-cherry-pick.py

@@ -12,7 +12,7 @@
 from kt.ktlib.ciq_helpers import (
     CIQ_cherry_pick_commit_standardization,
     CIQ_commit_exists_in_current_branch,
-    CIQ_find_fixes_in_mainline_current_branch,
+    CIQ_find_fixes_in_mainline_current_branch_unapplied,
     CIQ_find_matching_cve,
     CIQ_fixes_references,
     CIQ_get_full_hash,
@@ -251,7 +251,9 @@ def cherry_pick_fixes(
     Fixes: <sha>. If any, these will also be cherry picked with the ciq
     tag = cve-bf. If the tag was cve-pre, it stays the same.
     """
-    fixes_in_mainline = CIQ_find_fixes_in_mainline_current_branch(os.getcwd(), upstream_ref, sha)
+    fixes_in_mainline = CIQ_find_fixes_in_mainline_current_branch_unapplied(
+        repo=os.getcwd(), upstream_ref=upstream_ref, hash_=sha
+    )
 
     # Replace cve with cve-bf
     # Leave cve-pre and cve-bf as they are

kt/ktlib/ciq_helpers.py

@@ -229,35 +229,48 @@ def CIQ_hash_exists_in_ref(repo, pr_ref, hash_):
         return False
 
 
-def CIQ_commit_exists_in_branch(repo, pr_branch, upstream_hash_):
+def CIQ_commit_exists_in_branch(repo, pr_branch, upstream_hash_, interval=None):
     """
-    Return True if upstream_hash_ has been backported and it exists in the pr branch
+    Return True if upstream_hash_ has been backported and it exists in the pr branch.
+    If interval that consists of a pair of commit hashes is not None, we only
+    check the interval[0]..interval[1], otherwise the whole history of the pr_branch will be used.
     """
 
     # First check if the commit has been backported by CIQ
-    output = CIQ_run_git(repo, ["log", pr_branch, "--grep", "^commit " + upstream_hash_])
+    git_command = ["log", pr_branch, "--grep", "^commit " + upstream_hash_]
+    if interval:
+        oldest_commit, newest_commit = interval
+        git_command.append(f"{oldest_commit}..{newest_commit}")
+
+    output = CIQ_run_git(repo_path=repo, args=git_command)
     if output:
         return True
 
+    # If we are searching in the interval <oldest_commit>..<newest_commit>,
+    # we do not need to check way back if the commit came from upstream as it is
+    if interval:
+        return False
+
     # If it was not backported by CIQ, maybe it came from upstream as it is
-    return CIQ_hash_exists_in_ref(repo, pr_branch, upstream_hash_)
+    return CIQ_hash_exists_in_ref(repo=repo, pr_ref=pr_branch, hash_=upstream_hash_)
 
 
-def CIQ_commit_exists_in_current_branch(repo, upstream_hash_):
+def CIQ_commit_exists_in_current_branch(repo, upstream_hash_, interval=None):
     """
     Return True if upstream_hash_ has been backported and it exists in the current branch
     """
 
     current_branch = CIQ_get_current_branch(repo)
     full_upstream_hash = CIQ_get_full_hash(repo, upstream_hash_)
 
-    return CIQ_commit_exists_in_branch(repo, current_branch, full_upstream_hash)
+    return CIQ_commit_exists_in_branch(
+        repo=repo, pr_branch=current_branch, upstream_hash_=full_upstream_hash, interval=interval
+    )
 
 
-def CIQ_find_fixes_in_mainline(repo, pr_branch, upstream_ref, hash_):
+def CIQ_find_fixes_in_mainline(repo, upstream_ref, hash_):
     """
-    Return unique commits in upstream_ref that have Fixes: <N chars of hash_> in their message, case-insensitive,
-    if they have not been committed in the pr_branch.
+    Return unique commits in upstream_ref that have Fixes: <N chars of hash_> in their message, case-insensitive.
     Start from 12 chars and work down to 6, but do not include duplicates if already found at a longer length.
     Returns a list of tuples: (full_hash, display_string)
     """
@@ -295,17 +308,51 @@ def CIQ_find_fixes_in_mainline(repo, pr_branch, upstream_ref, hash_):
         for fix in fixes:
             for prefix in hash_prefixes:
                 if fix.lower().startswith(prefix.lower()):
-                    if not CIQ_commit_exists_in_branch(repo, pr_branch, full_hash):
-                        results.append((full_hash, display_string))
+                    results.append((full_hash, display_string))
                     break
 
     return results
 
 
-def CIQ_find_fixes_in_mainline_current_branch(repo, upstream_ref, hash_):
+def CIQ_filter_unapplied_commits(repo, pr_branch, commits, interval=None):
+    """
+    Receives a list of tuples: (full_hash, display_string)
+    Returns filtered list with commits that were not applied in the pr_branch.
+    If interval that consists of a pair of commit hashes is not None, we only
+    check the interval[0]..interval[1], otherwise the whole history of the pr_branch will be used.
+
+    Returns a list of tuples: (full_hash, display_string)
+    """
+
+    results = []
+    for commit in commits:
+        full_hash = commit[0]
+        if not CIQ_commit_exists_in_branch(repo=repo, pr_branch=pr_branch, upstream_hash_=full_hash, interval=interval):
+            results.append(commit)
+
+    return results
+
+
+def CIQ_find_fixes_in_mainline_unapplied(repo, pr_branch, upstream_ref, hash_, interval=None):
+    """
+    Return unique commits in upstream_ref that have Fixes: <N chars of hash_> in their message, case-insensitive,
+    if they have not been committed in the pr_branch.
+    If interval that consists of a pair of commit hashes is not None, we only
+    check the interval[0]..interval[1], otherwise the whole history of the pr_branch will be used.
+    Start from 12 chars and work down to 6, but do not include duplicates if already found at a longer length.
+    Returns a list of tuples: (full_hash, display_string)
+    """
+
+    fixes = CIQ_find_fixes_in_mainline(repo=repo, upstream_ref=upstream_ref, hash_=hash_)
+    return CIQ_filter_unapplied_commits(repo=repo, pr_branch=pr_branch, commits=fixes, interval=interval)
+
+
+def CIQ_find_fixes_in_mainline_current_branch_unapplied(repo, upstream_ref, hash_, interval=None):
     current_branch = CIQ_get_current_branch(repo)
 
-    return CIQ_find_fixes_in_mainline(repo, current_branch, upstream_ref, hash_)
+    return CIQ_find_fixes_in_mainline_unapplied(
+        repo=repo, pr_branch=current_branch, upstream_ref=upstream_ref, hash_=hash_, interval=interval
+    )
 
 
 def CIQ_reset_HEAD(repo):