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

Original issue: ctrliq/kernel-src-tree#1212 (comment)

In the above link, we had a weird case where commit X was reverted by commit Y,
but then our cve tracker picked the X commit again (why is irrelevant to this story).

check_kernel_commits.py did not detect any issues locally, but it did flag it
during the validate step when the PR was created. The discrepancy is due to
the fact that, locally, the whole pr/local branch is fetched, but during Pr
reviews, only the new commits are fetches, hence the history is way limited,
which is more than enough considering that the 'fixes:' (cve-bf) commits should
be applied after the commit of interest.

While the different results locally vs during Pr review make sense, we need
to catch issues like this locally, during development as well.

Because locally we cannot fetch just the new commits (we need the whole
history to make sure we cherry pick commits properly), check_kernel_commits
now checks the interval of commits it has to check.

And since we ran into this weird case described at the beginning, flag a
warnining in case a commit X has a cve-bf that was applied beforehand so that
the developer sees it and acts accordingly and not be confused during Pr
review.

Signed-off-by: Roxana Nicolescu <rnicolescu@ciq.com>

Author: roxanan1996

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):