Fix: direct commits still misclassified after #337 (compare mode)

release_notes_generator/data/miner.py

@@ -26,7 +26,7 @@
 from typing import Optional, Callable
 
 import semver
-from github import Github, GithubException
+from github import Github, GithubException, UnknownObjectException
 from github.GitRelease import GitRelease
 from github.Issue import Issue
 from github.PullRequest import PullRequest
@@ -43,8 +43,24 @@
 from release_notes_generator.utils.github_rate_limiter import GithubRateLimiter
 from release_notes_generator.utils.record_utils import get_id, parse_issue_id
 
-_PR_NUMBER_RE = re.compile(r"\(#(\d+)\)|Merge pull request #(\d+)")
+# dev note: these are GitHub-generated merge artifacts, so a match is a reliable signal that a commit
+#   is a PR merge/squash commit even if the PR itself couldn't be fetched (e.g. transient API error).
+_PR_MERGE_ARTIFACT_RE = re.compile(r"\(#(\d+)\)|Merge pull request #(\d+)")
+# dev note: 3rd alternative catches commits whose subject leads with a bare "#N" reference
+#   (e.g. "#1403 Fix thing"), a message style left as-is by some merge strategies (e.g. rebase-merge)
+#   that don't append GitHub's "(#N)"/"Merge pull request #N" boilerplate. Without it, such PRs are
+#   never looked up at all, so their commits can't be excluded as duplicates of the PR.
+#   Unlike _PR_MERGE_ARTIFACT_RE, this is only a candidate to try fetching - #N is a common commit
+#   convention for referencing an issue and is not proof the commit belongs to a real merged PR, so it
+#   must not by itself exclude a commit (see the SHA-based check in _handle_compare_mode).
+_PR_NUMBER_RE = re.compile(r"\(#(\d+)\)|Merge pull request #(\d+)|^#(\d+)\b")
 _COMPARE_COMMITS_MAX_RESULTS = 10_000
+# dev note: cap on how many PR-associated commit SHAs are logged at debug level, to keep verbose logs
+#   readable for large comparisons.
+_MAX_LOGGED_PR_COMMIT_SHAS = 50
+# dev note: cap on the per-commit "commit -> associated PRs" fallback lookup (see _handle_compare_mode)
+#   so a large batch of genuine direct commits can't trigger thousands of extra API calls.
+_MAX_DIRECT_COMMIT_PR_LOOKUPS = 200
 
 logger = logging.getLogger(__name__)
 
@@ -147,37 +163,80 @@ def _handle_compare_mode(self, repo: Repository, data: MinedData) -> None:
         data.compare_commit_shas = {c.sha for c in compare_commits}
         data.commits = {c: data.home_repository for c in compare_commits}
         pr_numbers = self._extract_pr_numbers_from_commits(compare_commits)
+        logger.debug("Compare mode: PR number(s) extracted from commit subjects: %s", sorted(pr_numbers))
         pulls: dict[PullRequest, Repository] = {}
         pr_commit_shas: set[str] = set()
         for number in sorted(pr_numbers):
-            pr = self._safe_call(repo.get_pull)(number)
-            if pr is not None:
-                # Store each PR with its source repository for downstream filtering and processing.
-                # In compare mode, all PRs come from home_repository; cross-repo is handled elsewhere.
-                pulls[pr] = data.home_repository
-                # dev note: pull.get_commits() returns all commits GitHub associates with the PR,
-                #   including sync-merge commits (base branch merged back into the PR branch) whose
-                #   messages don't match _PR_NUMBER_RE. Excluding them by SHA (rather than by message
-                #   pattern) prevents them being misclassified as stand-alone "direct commits".
-                #   merge_commit_sha is added separately: for a rebase-merge, get_commits() still
-                #   reports the pre-rebase SHAs, not the new SHA(s) landed on the base branch.
-                pr_commits = self._safe_call(lambda p=pr: list(p.get_commits()))()
-                if pr_commits is not None:
-                    pr_commit_shas.update(c.sha for c in pr_commits)
-                if pr.merge_commit_sha:
-                    pr_commit_shas.add(pr.merge_commit_sha)
-        data.pull_requests = pulls
+            pr = self._safe_call(lambda n=number: self._get_pull_ignoring_not_found(repo, n))()
+            if pr is None:
+                logger.debug("Compare mode: PR #%d could not be fetched; skipping.", number)
+                continue
+            self._register_pr_commit_shas(pr, pulls, pr_commit_shas, data.home_repository)
 
         # Only include commits that aren't already accounted for by a PR
         # (commits identified by PR, or belonging to a PR's commit list, are redundant with the PR itself)
         commits_without_pr: dict[GithubCommit, Repository] = {}
         for commit in compare_commits:
+            subject = commit.commit.message.splitlines()[0] if commit.commit.message else ""
             if commit.sha in pr_commit_shas:
+                logger.debug("Compare mode: commit %s ('%s') excluded, matched PR commit SHA.", commit.sha, subject)
                 continue
+            has_pr_ref = bool(_PR_MERGE_ARTIFACT_RE.search(subject))
+            if has_pr_ref:
+                logger.debug("Compare mode: commit %s ('%s') excluded, subject references a PR.", commit.sha, subject)
+                continue
+            commits_without_pr[commit] = data.home_repository
+
+        # dev note: some merged PR's commits never reference the PR number in any commit message at all
+        #   (e.g. "Set project version to 1.8.0") so they can't be found via _extract_pr_numbers_from_commits.
+        #   As a last-resort fallback, ask GitHub directly which merged PR(s) contain each remaining
+        #   candidate "direct commit" via the commit -> PRs association endpoint.
+        if len(commits_without_pr) > _MAX_DIRECT_COMMIT_PR_LOOKUPS:
+            logger.debug(
+                "Compare mode: %d commit(s) without a detected PR exceed the fallback lookup cap of %d; "
+                "skipping commit -> PR association fallback, some may still belong to a PR.",
+                len(commits_without_pr),
+                _MAX_DIRECT_COMMIT_PR_LOOKUPS,
+            )
+        else:
+            registered_pr_numbers = {p.number for p in pulls}
+            for commit in list(commits_without_pr):
+                associated_prs = self._safe_call(lambda c=commit: list(c.get_pulls()))()
+                for pr in associated_prs or []:
+                    # dev note: the association endpoint returns a "simple" PR representation without
+                    #   `merged`, so reading it lazily completes the object via a real API call - route
+                    #   it through _safe_call like every other GitHub-hitting call in this method.
+                    is_merged = self._safe_call(lambda p=pr: p.merged)()
+                    if not is_merged or pr.number in registered_pr_numbers:
+                        continue
+                    logger.debug(
+                        "Compare mode: commit %s is associated with merged PR #%d not found via commit subjects.",
+                        commit.sha,
+                        pr.number,
+                    )
+                    self._register_pr_commit_shas(pr, pulls, pr_commit_shas, data.home_repository)
+                    registered_pr_numbers.add(pr.number)
+                if commit.sha in pr_commit_shas:
+                    subject = commit.commit.message.splitlines()[0] if commit.commit.message else ""
+                    logger.debug(
+                        "Compare mode: commit %s ('%s') excluded, matched PR commit SHA via association fallback.",
+                        commit.sha,
+                        subject,
+                    )
+                    del commits_without_pr[commit]
+
+        for commit in commits_without_pr:
             subject = commit.commit.message.splitlines()[0] if commit.commit.message else ""
-            has_pr_ref = bool(_PR_NUMBER_RE.search(subject))
-            if not has_pr_ref:
-                commits_without_pr[commit] = data.home_repository
+            logger.debug("Compare mode: commit %s ('%s') classified as direct commit.", commit.sha, subject)
+
+        data.pull_requests = pulls
+        sorted_pr_commit_shas = sorted(pr_commit_shas)
+        logger.debug(
+            "Compare mode: total %d unique PR-associated commit SHA(s) (showing up to %d): %s",
+            len(sorted_pr_commit_shas),
+            _MAX_LOGGED_PR_COMMIT_SHAS,
+            sorted_pr_commit_shas[:_MAX_LOGGED_PR_COMMIT_SHAS],
+        )
 
         data.commits = commits_without_pr
         logger.info(
@@ -186,6 +245,52 @@ def _handle_compare_mode(self, repo: Repository, data: MinedData) -> None:
             len(data.pull_requests),
         )
 
+    def _register_pr_commit_shas(
+        self,
+        pr: PullRequest,
+        pulls: dict[PullRequest, Repository],
+        pr_commit_shas: set[str],
+        home_repository: Repository,
+    ) -> None:
+        """
+        Store `pr` alongside its home repository and add all commit SHAs GitHub associates with it
+        (via `get_commits()` plus `merge_commit_sha`) to `pr_commit_shas`.
+
+        dev note: `pull.get_commits()` returns all commits GitHub associates with the PR, including
+        sync-merge commits (base branch merged back into the PR branch) whose messages don't match
+        `_PR_NUMBER_RE`. Excluding them by SHA (rather than by message pattern) prevents them being
+        misclassified as stand-alone "direct commits". `merge_commit_sha` is added separately: for a
+        rebase-merge, `get_commits()` still reports the pre-rebase SHAs, not the new SHA(s) landed on
+        the base branch.
+        """
+        # In compare mode, all PRs come from home_repository; cross-repo is handled elsewhere.
+        pulls[pr] = home_repository
+        pr_commits = self._safe_call(lambda p=pr: list(p.get_commits()))()
+        pr_commit_sha_list = [c.sha for c in pr_commits] if pr_commits is not None else []
+        pr_commit_shas.update(pr_commit_sha_list)
+        if pr.merge_commit_sha:
+            pr_commit_shas.add(pr.merge_commit_sha)
+        logger.debug(
+            "Compare mode: PR #%d has %d commit(s) via get_commits() (showing up to %d) %s, merge_commit_sha=%s.",
+            pr.number,
+            len(pr_commit_sha_list),
+            _MAX_LOGGED_PR_COMMIT_SHAS,
+            pr_commit_sha_list[:_MAX_LOGGED_PR_COMMIT_SHAS],
+            pr.merge_commit_sha,
+        )
+
+    @staticmethod
+    def _get_pull_ignoring_not_found(repo: Repository, number: int) -> Optional[PullRequest]:
+        """
+        Fetch a PR by number, treating "not found" as an expected outcome (bare `#N` commit
+        references are as likely to point at an issue as at a PR) rather than an error worth a
+        full traceback in the logs.
+        """
+        try:
+            return repo.get_pull(number)
+        except UnknownObjectException:
+            return None
+
     def _validate_tag_exists(self, repo: Repository, tag: str) -> None:
         try:
             repo.get_git_ref(f"tags/{tag}")
@@ -600,7 +705,7 @@ def _extract_pr_numbers_from_commits(commits: list[GithubCommit]) -> set[int]:
         for commit in commits:
             subject = commit.commit.message.splitlines()[0] if commit.commit.message else ""
             for match in _PR_NUMBER_RE.finditer(subject):
-                number_str = match.group(1) or match.group(2)
+                number_str = match.group(1) or match.group(2) or match.group(3)
                 pr_numbers.add(int(number_str))
         return pr_numbers
 

release_notes_generator/record/factory/default_record_factory.py

@@ -44,6 +44,10 @@
 
 logger = logging.getLogger(__name__)
 
+# dev note: cap on how many matched commit SHAs are logged at debug level, to keep verbose logs
+#   readable for PRs with many commits.
+_MAX_LOGGED_COMMIT_SHAS = 50
+
 
 class DefaultRecordFactory(RecordFactory):
     """
@@ -92,6 +96,8 @@ def generate(self, data: MinedData) -> dict[str, Record]:
         logger.info("Registering direct commits to records...")
         for commit, repo in data.commits.items():
             if commit.sha not in self.__registered_commits:
+                subject = commit.commit.message.splitlines()[0] if commit.commit.message else ""
+                logger.debug("Direct commit registered: %s ('%s')", commit.sha, subject)
                 self._records[get_id(commit, repo)] = CommitRecord(commit)
 
         # dev note: now we have all PRs and commits registered to issues or as stand-alone records
@@ -136,6 +142,16 @@ def _register_pull_and_its_commits_to_issue(
             pr_commit_shas.add(pull.merge_commit_sha)
         related_commits = [c for c in data.commits if c.sha in pr_commit_shas]
         self.__registered_commits.update(c.sha for c in related_commits)
+        related_commit_shas = [c.sha for c in related_commits]
+        logger.debug(
+            "PR #%d: %d commit SHA(s) via get_commits() + merge_commit_sha, %d matched against mined commits "
+            "(showing up to %d): %s",
+            pull.number,
+            len(pr_commit_shas),
+            len(related_commits),
+            _MAX_LOGGED_COMMIT_SHAS,
+            related_commit_shas[:_MAX_LOGGED_COMMIT_SHAS],
+        )
 
         pr_repo = target_repository if target_repository is not None else data.home_repository